Re: CVS commit: src/sys/kern

[Andrew Doran <ad%netbsd.org@localhost>]

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.
 
>   - 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.

> @@ -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)

> +             struct mount *nmp;
> +
> +             /* If it fails, the file-system is [being] unmounted. */
> +             if (vfs_busy(mp, &mnp) != 0)
> +                     continue;
> +
>               fileassoc_table_run(mp, veriexec_hook,
>                   (fileassoc_cb_t)veriexec_file_dump, rarray);
> +
> +             vfs_unbusy(mp, false, &nmp);
>       }
>       mutex_exit(&mountlist_lock);