[9front] [PATCH] private /srv attach option

[9front] [PATCH] private /srv attach option

[Jacob Moody]

This patch add a 'p' attach option to srv to get a private session.
The sessions work similarly to #| sessions.

Attaching a private /srv does not effect future
attaches to '#s' without the private option.
I figure the global /srv can be explicitly
given up through chdev if desired.


---
diff b75e549126641108880a24a4ff0b38171eb1a856 uncommitted
--- a//sys/src/9/port/devsrv.c
+++ b//sys/src/9/port/devsrv.c
@@ -17,16 +17,23 @@
 	ulong	path;
 };

-static QLock	srvlk;
-static Srv	*srv;
-static int	qidpath;
+typedef struct Fid Fid;
+struct Fid
+{
+	int	ref;
+	QLock 	lk;
+	Srv 	*tail;
+	int 	qidpath;
+};

+static Fid	global;
+
 static Srv*
-srvlookup(char *name, ulong qidpath)
+srvlookup(Fid *f, char *name, ulong qidpath)
 {
 	Srv *sp;

-	for(sp = srv; sp != nil; sp = sp->link) {
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->path == qidpath || (name != nil && strcmp(sp->name, name) == 0))
 			return sp;
 	}
@@ -38,6 +45,7 @@
 {
 	Srv *sp;
 	Qid q;
+	Fid *f;

 	if(s == DEVDOTDOT){
 		devdir(c, c->qid, "#s", 0, eve, 0555, dp);
@@ -44,15 +52,17 @@
 		return 1;
 	}

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(name != nil)
-		sp = srvlookup(name, -1);
+		sp = srvlookup(f, name, -1);
 	else {
-		for(sp = srv; sp != nil && s > 0; sp = sp->link)
+		for(sp = f->tail; sp != nil && s > 0; sp = sp->link)
 			s--;
 	}
 	if(sp == nil || (name != nil && (strlen(sp->name) >= sizeof(up->genbuf)))) {
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		return -1;
 	}
 	mkqid(&q, sp->path, 0, QTFILE);
@@ -59,7 +69,7 @@
 	/* make sure name string continues to exist after we release lock */
 	kstrcpy(up->genbuf, sp->name, sizeof up->genbuf);
 	devdir(c, q, up->genbuf, 0, sp->owner, sp->perm, dp);
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	return 1;
 }

@@ -66,19 +76,45 @@
 static void
 srvinit(void)
 {
-	qidpath = 1;
+	global.qidpath = 1;
 }

 static Chan*
 srvattach(char *spec)
 {
-	return devattach('s', spec);
+	Chan *c;
+	Fid *f;
+
+	c = devattach('s', spec);
+	f = &global;
+	if(spec != nil && spec[0] == 'p'){
+		f = smalloc(sizeof *f);
+		f->qidpath = 1;
+		f->ref = 1;
+	}
+	c->aux = f;
+	return c;
 }

 static Walkqid*
 srvwalk(Chan *c, Chan *nc, char **name, int nname)
 {
-	return devwalk(c, nc, name, nname, 0, 0, srvgen);
+	Walkqid *wq;
+	Fid *f;
+
+	wq =  devwalk(c, nc, name, nname, 0, 0, srvgen);
+	if(wq == nil || wq->clone == nil || wq->clone == c)
+		return wq;
+
+	f = c->aux;
+	/* global doesn't ref count */
+	if(f == &global)
+		return wq;
+
+	qlock(&f->lk);
+	f->ref++;
+	qunlock(&f->lk);
+	return wq;
 }

 static int
@@ -91,11 +127,13 @@
 srvname(Chan *c)
 {
 	Srv *sp;
+	Fid *f;
 	char *s;

 	s = nil;
-	qlock(&srvlk);
-	for(sp = srv; sp != nil; sp = sp->link) {
+	f = &global;
+	qlock(&f->lk);
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->chan == c){
 			s = malloc(3+strlen(sp->name)+1);
 			if(s != nil)
@@ -103,7 +141,7 @@
 			break;
 		}
 	}
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	return s;
 }

@@ -111,6 +149,7 @@
 srvopen(Chan *c, int omode)
 {
 	Srv *sp;
+	Fid *f;
 	Chan *nc;

 	if(c->qid.type == QTDIR){
@@ -123,13 +162,14 @@
 		c->offset = 0;
 		return c;
 	}
-	qlock(&srvlk);
+	f = c->aux;
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}

-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil || sp->chan == nil)
 		error(Eshutdown);

@@ -144,7 +184,7 @@
 	nc = sp->chan;
 	incref(nc);

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	cclose(c);
@@ -155,6 +195,7 @@
 srvcreate(Chan *c, char *name, int omode, ulong perm)
 {
 	Srv *sp;
+	Fid *f;

 	if(openmode(omode) != OWRITE)
 		error(Eperm);
@@ -162,31 +203,32 @@
 	if(strlen(name) >= sizeof(up->genbuf))
 		error(Etoolong);

+	f = c->aux;
 	sp = smalloc(sizeof *sp);
 	kstrdup(&sp->name, name);
 	kstrdup(&sp->owner, up->user);

-	qlock(&srvlk);
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		free(sp->owner);
 		free(sp->name);
 		free(sp);
 		nexterror();
 	}
-	if(srvlookup(name, -1) != nil)
+	if(srvlookup(f, name, -1) != nil)
 		error(Eexist);

 	sp->perm = perm&0777;
-	sp->path = qidpath++;
+	sp->path = f->qidpath++;

 	c->qid.path = sp->path;
 	c->qid.type = QTFILE;

-	sp->link = srv;
-	srv = sp;
+	sp->link = f->tail;
+	f->tail = sp;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	c->flag |= COPEN;
@@ -199,16 +241,19 @@
 srvremove(Chan *c)
 {
 	Srv *sp, **l;
+	Fid *f;

 	if(c->qid.type == QTDIR)
 		error(Eperm);

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}
-	l = &srv;
+	l = &f->tail;
 	for(sp = *l; sp != nil; sp = *l) {
 		if(sp->path == c->qid.path)
 			break;
@@ -232,7 +277,15 @@
 	*l = sp->link;
 	sp->link = nil;

-	qunlock(&srvlk);
+	if(f != &global){
+		f->ref--;
+		if(f->ref <= 0)
+			free(f);
+		else
+			qunlock(&f->lk);
+	} else
+		qunlock(&f->lk);
+
 	poperror();

 	if(sp->chan != nil)
@@ -247,6 +300,7 @@
 {
 	char *strs;
 	Srv *sp;
+	Fid *f;
 	Dir d;

 	if(c->qid.type & QTDIR)
@@ -261,13 +315,15 @@
 	if(n == 0)
 		error(Eshortstat);

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}

-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil)
 		error(Enonexist);

@@ -286,7 +342,7 @@
 	if(d.mode != ~0UL)
 		sp->perm = d.mode & 0777;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	free(strs);
@@ -298,16 +354,32 @@
 static void
 srvclose(Chan *c)
 {
+	Fid *f;
+
 	/*
 	 * in theory we need to override any changes in removability
 	 * since open, but since all that's checked is the owner,
 	 * which is immutable, all is well.
 	 */
-	if(c->flag & CRCLOSE){
+	if((c->flag & COPEN) && (c->flag & CRCLOSE)){
 		if(waserror())
-			return;
+			goto ref;
+
 		srvremove(c);
 		poperror();
+	} else {
+
+ref:
+		f = c->aux;
+
+		if(f != &global){
+			qlock(&f->lk);
+			f->ref--;
+			if(f->ref <= 0)
+				free(f);
+			else
+				qunlock(&f->lk);
+		}
 	}
 }

@@ -322,6 +394,7 @@
 srvwrite(Chan *c, void *va, long n, vlong)
 {
 	Srv *sp;
+	Fid *f;
 	Chan *c1;
 	int fd;
 	char buf[32];
@@ -334,15 +407,17 @@

 	c1 = fdtochan(fd, -1, 0, 1);	/* error check and inc ref */

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()) {
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		cclose(c1);
 		nexterror();
 	}
 	if(c1->qid.type & QTAUTH)
 		error("cannot post auth file in srv");
-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil)
 		error(Enonexist);

@@ -351,7 +426,7 @@

 	sp->chan = c1;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();
 	return n;
 }
@@ -381,11 +456,13 @@
 srvrenameuser(char *old, char *new)
 {
 	Srv *sp;
+	Fid *f;

-	qlock(&srvlk);
-	for(sp = srv; sp != nil; sp = sp->link) {
+	f = &global;
+	qlock(&f->lk);
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->owner != nil && strcmp(old, sp->owner) == 0)
 			kstrdup(&sp->owner, new);
 	}
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 }

Re: [9front] [PATCH] private /srv attach option

[ori]

Quoth Jacob Moody <moody@mail.posixcafe.org>:
> Attaching a private /srv does not effect future
> attaches to '#s' without the private option.
> I figure the global /srv can be explicitly
> given up through chdev if desired.

Unfortunately, that doesn't recurse; I can't clear
out /srv, and add stuff, and let a child clear out
*its* /srv.

Re: [9front] [PATCH] private /srv attach option

[Jacob Moody]

On 5/30/22 08:44, ori@eigenstate.org wrote:
> Unfortunately, that doesn't recurse; I can't clear
> out /srv, and add stuff, and let a child clear out
> *its* /srv.
> 

Yes this was a purposeful design decision. If you notice
this code is entirely contained to devsrv itself. If you wanted
'forkable' /srv ala other process groups it only stands to reason
you would add a process group for it and add RF* flags for it.
But I was under the impression that this approach was not desired
or we would have merged in the ANTS work much earlier.

I personally do not see a better way of accomplishing
a 'forkable' /srv without stashing state in to
a process group. I want to avoid that,
and the current implementation reflects that.

> also, maybe it's time to sit down and think about
> where this all goes, rather than tossing random
> diffs over the fence.
>

Now correct me if I am wrong, but I think we tend to like
changes that are made with the implementation in mind.
I "throw a diff over the fence" so I can get an idea of how the
implementation works. I don't like ordaining how something
should work without an idea of how it works currently.

> - What is the current state of locking down... things?

Same as before except we can toss out drivers now.

> - Why is the current state insufficient?

The goal of this patch is to allow a way of keeping the capabilities
that /srv gives, while having it isolated from third parties.
It provides a middle ground between "global" state and "can't
be used at all". I wouldn't call the current state in relation
to this patch 'insufficient', I just think we can do better. Perhaps
we don't need to do better.

> - Are there places where the current state should have
>   been used, but wasn't because of its flaws?

I dont think this question applies? This isn't an iteration
on chdev to hack around an edge case. This is just the idea
that providing the middle ground for /srv as described above could be
desirable.

> - What exactly is the approach to securing things that
>   we're trying to approach?
>

I would like to describe a processes capabilities on the
system as the set of files it has access to. We have a mechanism
for modifying the set of files a process has access to through
namespaces. So it stands to reason that modification of capabilities
should be done through namespace operations. That's the core of my
thoughts.

> A soup of new features of 'rfork M' quality is a problem;
> let's figure out where we're going.
>

Ouch.

For the record I did discuss some of these /srv ideas
in another thread, I believe the it was the one for git/serve to use chdev.
Nobody seemed interested in discussing it then.

But point taken, I wont think about touching code until I fill out
my bikeshedding bingo card first.

Re: [9front] [PATCH] private /srv attach option

[ori]

Quoth Jacob Moody <moody@mail.posixcafe.org>:
> 
> > A soup of new features of 'rfork M' quality is a problem;
> > let's figure out where we're going.
> >
> 
> Ouch.
> 
> For the record I did discuss some of these /srv ideas
> in another thread, I believe the it was the one for git/serve to use chdev.
> Nobody seemed interested in discussing it then.
> 

in isolation, this change seems.. fine, I guess. It does
something useful, though it's got some inelegance around
needing a global view to get a clean /srv, which means
that a restricted /srv can't get a clean /srv. It's also
encoded into the interface, so we can't easily fix it
later.

This may or may not matter, I don't know.

the gripe I have is that I know you're aiming turn namespaces
into useful security boundaries, as well as providing other
ways to ratchet down permissions. But I haven't seen a summary
of the changes you feel are needed, the places and ways that
they'd be used, and a diversity of examples that demonstrate
the changes are parsimonious and sufficient.

What is the end goal, and what should it be?

Let's not design in a vaccuum.

Re: [9front] [PATCH] private /srv attach option

[hiro]

it's not moody's fault that the srv design sucks.

sure there's an interesting question in how could we do without srv,
but nobody seems to be answering that one.

so we get incremental improvements.

blame bell-labs.

Re: [9front] [PATCH] private /srv attach option

[Jacob Moody]

I spent the better half of yesterday thinking about how /srv fits into
the current work.

First lets talk about how we go about building an isolated namespace
as is. The general idea is to only use the root folders provided by #/,
then selectively binding in the necessities from the real root, then dropping
any reference to the real root from the namespace. This is roughly how the
current tcp80.namespace works, but this solution does not scale too well
to programs that require a lot of file system state.

An example is generally anything required out of /lib and/or /sys, these
are not folders given by #/ and binding in the entirety of them from
the real root isn't right either. So how do we setup the correct
state in /?

One way is to build a skeleton hierarchy stashed somewhere that can
be unioned on to /, and used as targets for binds from the real root
in the namespace file. This works pretty well for third party programs,
an example for this would be something like werc.

but shipping a skeleton for programs in tree sucks. So what else
do we have? Well there is always mntgen. But mntgen has some
issues:

1. There is some data leakage about what one client using through what folders show up when listing the root.
	Likely a somewhat easy fix
2. Mntgen only makes up directories at the root, we would want to extend this to child directories
	Also sounds like a doable fix
3. Mntgen is a userspace program, there is no facility for getting a fresh one in namespace files. You have to have
one sitting in /srv somewhere for it to be useful.

This last point is interesting and one I wanted to discuss more of. We could just provide a 'mntgen' command
to namespace files to get a private mount of mntgen. But you wouldn't want to do this if you are rebuilding
the namespace on each connection. A program wishing to do this could setup a mntgen/ramfs with the correct skeleton
in a private /srv to prevent having to share it with the rest of the system.

Now is the restriction to not being able to recurse through subsequent private /srv's once blocking
the global /srv an issue? I dont really think it is. I explicitly do not want chdev to proliferate
to every program on the system like openBSD's pledge. It is designed to be used at the very edge
of the process tree, for leaf processes that are handling potentially malicious input. In part due to
the fact that we never allow a process, or it's children, to regain access to something that has been lost.
This is to say that a program should know where it's process tree ends, and what capabilities it's children
will need. So you either end up with:

bind -c '#sp' /srv
chdev -r 's'

in the 'setup' phase of the parent if you know your children will never need to make another further private /srv.
If you do need to create a further private /srv, then simply kick that code snippet down the process
tree to the last place you need to substantiate the private /srv. If you know the child will want a private /srv
but the parent will not. Fork the child with RFNAMEG, then do 'chdev -r s' in the parent after the fork call.

This is only an issue if you:

1. Don't know what capabilities your children want.
	At which point chdev isn't very helpful to begin with.
2. Conditionally making a private /srv after you've begun reading potentially malicious input
	I think you kinda get what you aught to here.

I'm just trying to really rack my head for a scenario where you need to leave the global /srv on the table
because you don't know if your children will want another private /srv. If others have examples, please
let me know. I am pushing more because I _want_ it to be good enough for devsrv to just keep track of
sessions itself. I dont expect devsrv to be the last device we may want private sessions for, and growing
a new process group per device is not ideal. But maybe we can do a bit better then this,
I am going to poke around with an in-tree way (ctl file or similar) to see if that shakes out to being
a bit nicer. But I will say I do prefer the semantics of an attach option for getting a different 'tree'
of /srv, it feels right.

My grand plan is as you laid out, I want to turn namespaces in to security boundaries, and for the most part
we're there now. Chdev allows namespaces to be security boundaries.

The tricky part that we're trying to decide now is what tools do we want to give in order to make these types of namespaces
easier to build. It is much harder to lay out a plan for this. This mail serves to attempt to lay out what I have in my head
in this regard. The programs we choose to grow new features is dependent on their implementation quirks, such that our new use
case does not impose too much on to the existing code. So a lot of it turns in to 'see what the low hanging fruits are' and reflecting
on how/if these modifications produce a worthwhile tool.

So there's the big thought dump, pick it apart as desired.


Thanks,
moody

Re: [9front] [PATCH] private /srv attach option

[Jacob Moody]

On 5/31/22 09:09, Jacob Moody wrote:
> I explicitly do not want chdev to proliferate to every program on the system like openBSD's pledge. It is designed to be used at the very edge

To clarify, I dont think this is something we should absolutely avoid. I just dont want to feel like we need to touch every program on the system.
But I have put in some thought to how we would go about this if we wanted to more closely emulate it:

* chdev could grow a -c flag for setting permissions of it's child.
	Child would be defined as the direct RFCNAMEG namespace descendant. Default
	would be to inherit parents unless it was been explicitly set otherwise.
	This allows a child to have more capabilities then the parent.

* /dev/drivers could be moved to it's own driver
	This would make it a bit nicer for removing the ability to further modify
	the set of capabilities of it's child. It being bundled in the devcons
	kind of muddies this.


Some more food for thought.

Thanks,
moody

Re: [9front] [PATCH] private /srv attach option

[Jacob Moody]

Another iteration on this:

Scrapped the attach option.
This uses a /new child directory for getting a fresh instance.
The new instance is alloted on walks to this dir, but due
to our fid clone conventions you can safely interact
with the file without worrying about it nuking your /srv.

Only other change from the previous patch is that we ensure
that each private session has unique qid paths.

This makes the convention for getting a private /srv:

bind /srv/new /srv

This patch might need a bit more tidying, but in general I am
pretty happy with this interface.


Thanks,
moody

----
diff 1b5ea51ee1203952900fafc0def48985d900f7a7 uncommitted
--- a//sys/src/9/port/devsrv.c
+++ b//sys/src/9/port/devsrv.c
@@ -5,6 +5,7 @@
 #include	"fns.h"
 #include	"../port/error.h"

+#include	"netif.h"

 typedef struct Srv Srv;
 struct Srv
@@ -17,16 +18,29 @@
 	ulong	path;
 };

-static QLock	srvlk;
-static Srv	*srv;
-static int	qidpath;
+typedef struct Fid Fid;
+struct Fid
+{
+	int	ref;
+	QLock 	lk;
+	Srv 	*tail;
+	ulong 	nextpath;
+};

+struct {
+	QLock;
+	ulong path;
+} sessions;
+
+static Fid	global;
+
 static Srv*
-srvlookup(char *name, ulong qidpath)
+srvlookup(Fid *f, char *name, ulong qidpath)
 {
 	Srv *sp;

-	for(sp = srv; sp != nil; sp = sp->link) {
+	qidpath = NETTYPE(qidpath);
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->path == qidpath || (name != nil && strcmp(sp->name, name) == 0))
 			return sp;
 	}
@@ -38,28 +52,57 @@
 {
 	Srv *sp;
 	Qid q;
+	Fid *f;
+	ulong id;

 	if(s == DEVDOTDOT){
-		devdir(c, c->qid, "#s", 0, eve, 0555, dp);
+		devdir(c, c->qid, "#s", 0, eve, 0555|DMDIR, dp);
 		return 1;
 	}

-	qlock(&srvlk);
-	if(name != nil)
-		sp = srvlookup(name, -1);
-	else {
-		for(sp = srv; sp != nil && s > 0; sp = sp->link)
+	f = c->aux;
+	qlock(&f->lk);
+	id = NETID(c->qid.path);
+
+	if(name != nil){
+		if(strcmp(name, "new") == 0){
+			if(f != &global && --f->ref == 0){
+				qunlock(&f->lk);
+				free(f);
+			} else
+				qunlock(&f->lk);
+			f = smalloc(sizeof *f);
+			qlock(&sessions);
+			id = ++sessions.path;
+			qunlock(&sessions);
+			f->ref = 1;
+			f->nextpath = 2;
+			mkqid(&q, NETQID(id, 1), 0, QTDIR);
+			devdir(c, q, "new", 0, eve, 0555|DMDIR, dp);
+			c->aux = f;
+			return 1;
+		}
+		sp = srvlookup(f, name, -1);
+	} else {
+		if(s == 0){
+			mkqid(&q, NETQID(id, 1), 0, QTDIR);
+			devdir(c, q, "new", 0, eve, 0555|DMDIR, dp);
+			qunlock(&f->lk);
+			return 1;
+		}
+		s -= 1;
+		for(sp = f->tail; sp != nil && s > 0; sp = sp->link)
 			s--;
 	}
 	if(sp == nil || (name != nil && (strlen(sp->name) >= sizeof(up->genbuf)))) {
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		return -1;
 	}
-	mkqid(&q, sp->path, 0, QTFILE);
+	mkqid(&q, NETQID(id, sp->path), 0, QTFILE);
 	/* make sure name string continues to exist after we release lock */
 	kstrcpy(up->genbuf, sp->name, sizeof up->genbuf);
 	devdir(c, q, up->genbuf, 0, sp->owner, sp->perm, dp);
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	return 1;
 }

@@ -66,19 +109,38 @@
 static void
 srvinit(void)
 {
-	qidpath = 1;
+	global.nextpath = 2;
 }

 static Chan*
 srvattach(char *spec)
 {
-	return devattach('s', spec);
+	Chan *c;
+
+	c = devattach('s', spec);
+	c->aux = &global;
+	return c;
 }

 static Walkqid*
 srvwalk(Chan *c, Chan *nc, char **name, int nname)
 {
-	return devwalk(c, nc, name, nname, 0, 0, srvgen);
+	Walkqid *wq;
+	Fid *f;
+
+	wq =  devwalk(c, nc, name, nname, 0, 0, srvgen);
+	if(wq == nil || wq->clone == nil || wq->clone == c)
+		return wq;
+
+	f = c->aux;
+	/* global doesn't ref count */
+	if(f == &global)
+		return wq;
+
+	qlock(&f->lk);
+	f->ref++;
+	qunlock(&f->lk);
+	return wq;
 }

 static int
@@ -91,11 +153,13 @@
 srvname(Chan *c)
 {
 	Srv *sp;
+	Fid *f;
 	char *s;

 	s = nil;
-	qlock(&srvlk);
-	for(sp = srv; sp != nil; sp = sp->link) {
+	f = &global;
+	qlock(&f->lk);
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->chan == c){
 			s = malloc(3+strlen(sp->name)+1);
 			if(s != nil)
@@ -103,7 +167,7 @@
 			break;
 		}
 	}
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	return s;
 }

@@ -111,6 +175,7 @@
 srvopen(Chan *c, int omode)
 {
 	Srv *sp;
+	Fid *f;
 	Chan *nc;

 	if(c->qid.type == QTDIR){
@@ -123,13 +188,14 @@
 		c->offset = 0;
 		return c;
 	}
-	qlock(&srvlk);
+	f = c->aux;
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}

-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil || sp->chan == nil)
 		error(Eshutdown);

@@ -144,7 +210,7 @@
 	nc = sp->chan;
 	incref(nc);

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	cclose(c);
@@ -155,6 +221,7 @@
 srvcreate(Chan *c, char *name, int omode, ulong perm)
 {
 	Srv *sp;
+	Fid *f;

 	if(openmode(omode) != OWRITE)
 		error(Eperm);
@@ -162,31 +229,32 @@
 	if(strlen(name) >= sizeof(up->genbuf))
 		error(Etoolong);

+	f = c->aux;
 	sp = smalloc(sizeof *sp);
 	kstrdup(&sp->name, name);
 	kstrdup(&sp->owner, up->user);

-	qlock(&srvlk);
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		free(sp->owner);
 		free(sp->name);
 		free(sp);
 		nexterror();
 	}
-	if(srvlookup(name, -1) != nil)
+	if(srvlookup(f, name, -1) != nil)
 		error(Eexist);

 	sp->perm = perm&0777;
-	sp->path = qidpath++;
+	sp->path = f->nextpath++;

-	c->qid.path = sp->path;
+	c->qid.path = NETQID(NETID(c->qid.path), sp->path);
 	c->qid.type = QTFILE;

-	sp->link = srv;
-	srv = sp;
+	sp->link = f->tail;
+	f->tail = sp;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	c->flag |= COPEN;
@@ -199,18 +267,21 @@
 srvremove(Chan *c)
 {
 	Srv *sp, **l;
+	Fid *f;

 	if(c->qid.type == QTDIR)
 		error(Eperm);

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}
-	l = &srv;
+	l = &f->tail;
 	for(sp = *l; sp != nil; sp = *l) {
-		if(sp->path == c->qid.path)
+		if(sp->path == NETTYPE(c->qid.path))
 			break;
 		l = &sp->link;
 	}
@@ -232,7 +303,15 @@
 	*l = sp->link;
 	sp->link = nil;

-	qunlock(&srvlk);
+	if(f != &global){
+		f->ref--;
+		if(f->ref <= 0)
+			free(f);
+		else
+			qunlock(&f->lk);
+	} else
+		qunlock(&f->lk);
+
 	poperror();

 	if(sp->chan != nil)
@@ -247,6 +326,7 @@
 {
 	char *strs;
 	Srv *sp;
+	Fid *f;
 	Dir d;

 	if(c->qid.type & QTDIR)
@@ -261,13 +341,15 @@
 	if(n == 0)
 		error(Eshortstat);

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()){
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		nexterror();
 	}

-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil)
 		error(Enonexist);

@@ -286,7 +368,7 @@
 	if(d.mode != ~0UL)
 		sp->perm = d.mode & 0777;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();

 	free(strs);
@@ -298,16 +380,32 @@
 static void
 srvclose(Chan *c)
 {
+	Fid *f;
+
 	/*
 	 * in theory we need to override any changes in removability
 	 * since open, but since all that's checked is the owner,
 	 * which is immutable, all is well.
 	 */
-	if(c->flag & CRCLOSE){
+	if((c->flag & COPEN) && (c->flag & CRCLOSE)){
 		if(waserror())
-			return;
+			goto ref;
+
 		srvremove(c);
 		poperror();
+	} else {
+
+ref:
+		f = c->aux;
+
+		if(f != &global){
+			qlock(&f->lk);
+			f->ref--;
+			if(f->ref <= 0)
+				free(f);
+			else
+				qunlock(&f->lk);
+		}
 	}
 }

@@ -322,6 +420,7 @@
 srvwrite(Chan *c, void *va, long n, vlong)
 {
 	Srv *sp;
+	Fid *f;
 	Chan *c1;
 	int fd;
 	char buf[32];
@@ -334,15 +433,17 @@

 	c1 = fdtochan(fd, -1, 0, 1);	/* error check and inc ref */

-	qlock(&srvlk);
+	f = c->aux;
+
+	qlock(&f->lk);
 	if(waserror()) {
-		qunlock(&srvlk);
+		qunlock(&f->lk);
 		cclose(c1);
 		nexterror();
 	}
 	if(c1->qid.type & QTAUTH)
 		error("cannot post auth file in srv");
-	sp = srvlookup(nil, c->qid.path);
+	sp = srvlookup(f, nil, c->qid.path);
 	if(sp == nil)
 		error(Enonexist);

@@ -351,7 +452,7 @@

 	sp->chan = c1;

-	qunlock(&srvlk);
+	qunlock(&f->lk);
 	poperror();
 	return n;
 }
@@ -381,11 +482,13 @@
 srvrenameuser(char *old, char *new)
 {
 	Srv *sp;
+	Fid *f;

-	qlock(&srvlk);
-	for(sp = srv; sp != nil; sp = sp->link) {
+	f = &global;
+	qlock(&f->lk);
+	for(sp = f->tail; sp != nil; sp = sp->link) {
 		if(sp->owner != nil && strcmp(old, sp->owner) == 0)
 			kstrdup(&sp->owner, new);
 	}
-	qunlock(&srvlk);
+	qunlock(&f->lk);
 }

Re: [9front] [PATCH] private /srv attach option

[cinap_lenrek]

small bugs.

readstr() can error, so you need to put a waserror() to do
the runlock(b).

i'm a bit confused about the boardclunk().

as i see it, the original Board *b would never be unlocked
if the loop condition was true.

as the loop at the bottom changes b to the parent.

then the final wunlock() at the bottom would unlock the
wrong thing?

also, the lock order seems strange. we'r here locking from
the bottom up to the root.

as long as we always lock from the bottom to up, we'r
fine, but if we have any case where we acquired parent
lock, and then try to aquire a child lock we have
a lock order violation that can lead to deadlock.

sorry, didnt have time before to code review.

--
cinap

Re: [9front] [PATCH] private /srv attach option

[ori]

Quoth Jacob Moody <moody@mail.posixcafe.org>:
> Attaching a private /srv does not effect future
> attaches to '#s' without the private option.
> I figure the global /srv can be explicitly
> given up through chdev if desired.

also, maybe it's time to sit down and think about
where this all goes, rather than tossing random
diffs over the fence.

- What is the current state of locking down... things?
- Why is the current state insufficient?
- Are there places where the current state should have
  been used, but wasn't because of its flaws?
- What exactly is the approach to securing things that
  we're trying to approach?

A soup of new features of 'rfork M' quality is a problem;
let's figure out where we're going.

Re: [9front] [PATCH] private /srv attach option

[ori]

Quoth Jacob Moody <moody@mail.posixcafe.org>:
> This patch add a 'p' attach option to srv to get a private session.
> The sessions work similarly to #| sessions.
> 
> Attaching a private /srv does not effect future
> attaches to '#s' without the private option.
> I figure the global /srv can be explicitly
> given up through chdev if desired.
> 
> 

For those following along, https://shithub.us is now running
an updated version of this code, along with the patch to grow
the size of args, and a slightly tweaked version of auth/box.

sandboxing is now done with:

	#!/bin/auth/box -r/mnt -r/usr/git -r/sys/lib/tcp80 -r/usr/web -r/sys/lib/shithub -r/n -r/dev -eM -s
	
	<[3]/srv/clone{
		d=`{<[0=3]read}
		bind /srv/$d /srv
		# ugly, but we don't want to leak the clone fd into
		# procs that may stick around.
		<[3=0]{
			rfork n
			bind /usr/web /mnt/static
			execfs -m /usr/web /sys/lib/tcp80/gitrules
			bind /mnt/static /usr/web/static
			rfork n
			cd /
			exec /bin/tcp80
		}
	}

using auth/box as a #! line is quite nice, though even
with the `-s` option, the lines tend to get a bit long.