Re: CVS commit: src/sys/kern

[Elad Efrat <elad%NetBSD.org@localhost>]

Andrew Doran wrote:
> On Thu, Apr 23, 2009 at 12:18:24AM +0300, Elad Efrat wrote:
>
> > While looking around, a few questions came up:
> >
> >   - Take a look at:
> >
> >         http://nxr.netbsd.org/source/xref/sys/kern/vfs_subr.c#2120
> >
> >     It seems that if vnalloc() fails, we're not releasing the
> >     mountlist_mutex and not doing vfs_unbusy() -- is this a problem?
> >     (this holds for the other error paths in this function too)
>
> vnalloc(mp) where mp!=NULL should never fail so it would be better to
> replace the if() with a KASSERT(mp != NULL) to document the assumption.
> So it's a bug but one that should never fire.

Don't you mean KASSERT(mvp != NULL)? if so, I've done it (see attached
diff, vfs_subr.c.diff) and added a comment above why it's there.

> >   - It seems that the two functions that use CIRCLEQ_FOREACH() to
> >     traverse the mountlist don't vfs_busy()/vfs_unbusy(). Is this okay
> >     because they're supposed to be "light" users? (if yes, where do we
> >     draw the line between light/heavy uses? :)
>
> It see only one that's called at runtime, vfs_getvfs(). It is a `light'
> user, right (however note the XXX comment above).
>
> >   - I'm not sure how much of a problem it is, but since we're dropping
> >     the mountlist lock in the middle, doesn't it mean it's possible to
> >     insert elements into the mountlist "before" elements we've already
> >     passed? (in other words, perhaps we should make the mountlist KPI
> >     only append to the list, rather than exposing it as a CIRCLEQ?)
>
> I can't answer that for veriexec because I don't know the code and don't
> have time to look at it right now. When looking at something like this you
> have to consider:
>
> - what can the user do behind our back
> - what is a reasonable outcome given interference
> - most importantly, what is the user asking for - what is expected
>
> What's expected of veriexec_flush() in this case? If it must flush _all_
> state out, even new state that appears during the flush, then I think it's
> better to restart the traversal of mountlist until we run out of things to
> do (walk off the end of the list).
>
> However it's rarely the case the case that we need to do stuff like this,
> because most actions are user-initiated and the intent is to apply them to
> everything that has happened before that point. Consider that once your
> routine returns (eg veriexec_flush()) the user can go and build up new state
> unless you have some other kind of gating/serialization. To repeat I don't
> know what the intent is here.

Right. I'll have to look further into whether I think we should lock the
tables throughout these operations. For now I think we're fine though.

> > @@ -1539,8 +1539,16 @@ veriexec_dump(struct lwp *l, prop_array_
> >  
> >  	mutex_enter(&mountlist_lock);
> >         CIRCLEQ_FOREACH(mp, &mountlist, mnt_list) {
>
>         for (mp = CIRCLEQ_FIRST(&mountlist); mp != NULL; mp = nmp)

Of course, otherwise what good nmp is? :) fixed diff attached.

Thanks for your input,

-e.

Index: vfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
retrieving revision 1.374
diff -u -p -r1.374 vfs_subr.c
--- vfs_subr.c  22 Apr 2009 22:57:09 -0000      1.374
+++ vfs_subr.c  23 Apr 2009 10:28:29 -0000
@@ -2125,10 +2125,9 @@ sysctl_kern_vnode(SYSCTLFN_ARGS)
                }
                savebp = bp;
                /* Allocate a marker vnode. */
-               if ((mvp = vnalloc(mp)) == NULL) {
-                       sysctl_relock();
-                       return (ENOMEM);
-               }
+               mvp = vnalloc(mp);
+               /* Should never fail for mp != NULL */
+               KASSERT(mvp != NULL);
                mutex_enter(&mntvnode_lock);
                for (vp = TAILQ_FIRST(&mp->mnt_vnodelist); vp; vp = 
vunmark(mvp)) {
                        vmark(mvp, vp);

Index: kern_verifiedexec.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_verifiedexec.c,v
retrieving revision 1.113
diff -u -p -r1.113 kern_verifiedexec.c
--- kern_verifiedexec.c 20 Apr 2009 22:09:54 -0000      1.113
+++ kern_verifiedexec.c 23 Apr 2009 10:23:10 -0000
@@ -1535,12 +1535,18 @@ veriexec_file_dump(struct veriexec_file_
 int
 veriexec_dump(struct lwp *l, prop_array_t rarray)
 {
-       struct mount *mp;
+       struct mount *mp, *nmp;
 
        mutex_enter(&mountlist_lock);
-       CIRCLEQ_FOREACH(mp, &mountlist, mnt_list) {
+       for (mp = CIRCLEQ_FIRST(&mountlist); mp != NULL; mp = nmp) {
+               /* If it fails, the file-system is [being] unmounted. */
+               if (vfs_busy(mp, &nmp) != 0)
+                       continue;
+
                fileassoc_table_run(mp, veriexec_hook,
                    (fileassoc_cb_t)veriexec_file_dump, rarray);
+
+               vfs_unbusy(mp, false, &nmp);
        }
        mutex_exit(&mountlist_lock);
 
@@ -1550,16 +1556,22 @@ veriexec_dump(struct lwp *l, prop_array_
 int
 veriexec_flush(struct lwp *l)
 {
-       struct mount *mp;
+       struct mount *mp, *nmp;
        int error = 0;
 
        mutex_enter(&mountlist_lock);
-       CIRCLEQ_FOREACH(mp, &mountlist, mnt_list) {
+       for (mp = CIRCLEQ_FIRST(&mountlist); mp != NULL; mp = nmp) {
                int lerror;
 
+               /* If it fails, the file-system is [being] unmounted. */
+               if (vfs_busy(mp, &nmp) != 0)
+                       continue;
+
                lerror = veriexec_table_delete(l, mp);
                if (lerror && lerror != ENOENT)
                        error = lerror;
+
+               vfs_unbusy(mp, false, &nmp);
        }
        mutex_exit(&mountlist_lock);