[PATCH] devfs v194 available

[PATCH] devfs v194 available

[Richard Gooch]

  Hi, all. Version 194 of my devfs patch is now available from:
http://www.atnf.csiro.au/~rgooch/linux/kernel-patches.html
The devfs FAQ is also available here.

Patch directly available from:
ftp://ftp.??.kernel.org/pub/linux/kernel/people/rgooch/v2.4/devfs-patch-current.gz

AND:
ftp://ftp.atnf.csiro.au/pub/people/rgooch/linux/kernel-patches/v2.4/devfs-patch-current.gz

This is against 2.4.11-pre6. Highlights of this release:

- Fixed overrun in <devfs_link> by removing function (not needed)

- Updated README from master HTML file

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] devfs v194 available

[Alexander Viro]

On Tue, 9 Oct 2001, Richard Gooch wrote:

>   Hi, all. Version 194 of my devfs patch is now available from:
> http://www.atnf.csiro.au/~rgooch/linux/kernel-patches.html
> The devfs FAQ is also available here.
> 
> Patch directly available from:
> ftp://ftp.??.kernel.org/pub/linux/kernel/people/rgooch/v2.4/devfs-patch-current.gz
> 
> AND:
> ftp://ftp.atnf.csiro.au/pub/people/rgooch/linux/kernel-patches/v2.4/devfs-patch-current.gz
> 
> This is against 2.4.11-pre6. Highlights of this release:
> 
> - Fixed overrun in <devfs_link> by removing function (not needed)

... doesn't fix _under_run in try_modload() (see what happens if namelen is
255 and parent is devfs root)

... doesn't fix the deadlock introduced into -pre6 in place of symlink
races. That

    /*  Need to follow the link: this is a stack chomper  */
    down_read (&symlink_rwsem);
    retval = curr->registered ?
        search_for_entry (parent, curr->u.symlink.linkname,
                          curr->u.symlink.length, FALSE, FALSE, NULL,
                          TRUE) : NULL;
    up_read (&symlink_rwsem);

is a fairly bad idea.  Think what happens if somebody else tries to
acquire symlink_rwsem for write between two calls of down_read() in
that recursion.

Re: [PATCH] devfs v194 available

[Richard Gooch]

Alexander Viro writes:
> ... doesn't fix _under_run in try_modload() (see what happens if
> namelen is 255 and parent is devfs root)

What underrun?
How can this be a problem? Before I use pos, I have the following
check:
    if (namelen >= STRING_LENGTH) return -ENAMETOOLONG;

so the maximum value that namelen can be is STRING_LENGTH-1. Thus we
have:
    pos = STRING_LENGTH - (STRING_LENGTH - 1) - 1;
->  pos = 0;

> ... doesn't fix the deadlock introduced into -pre6 in place of symlink
> races. That
> 
>     /*  Need to follow the link: this is a stack chomper  */
>     down_read (&symlink_rwsem);
>     retval = curr->registered ?
>         search_for_entry (parent, curr->u.symlink.linkname,
>                           curr->u.symlink.length, FALSE, FALSE, NULL,
>                           TRUE) : NULL;
>     up_read (&symlink_rwsem);
> 
> is a fairly bad idea.  Think what happens if somebody else tries to
> acquire symlink_rwsem for write between two calls of down_read() in
> that recursion.

Where is the problem? After the first call to down_read(), anyone
calling down_write() will block, which is fine. Then the second call
to down_read() should still work, right? Once the recursion unwinds,
the semaphore will be released and whoever is waiting on down_write()
will unblock.

There should be no code paths where there is recursion between
down_read() and down_write() (otherwise we *would* have a deadlock).

Ah, shit. I just checked the rwsem implementation. It seems that once
we do a down_write() (even if that blocks because someone else has a
down_read() already), subsequent down_read() calls will block until
the writer is granted access and then does up_write(). Damn. It would
have been good for this to be documented somewhere. Those are the
kinds of traps that should be mentioned in the header file.

OK: is there a variant of rwsem which is "unfair" (i.e. readers can
starve writers indefinately)?

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca

Re: [PATCH] devfs v194 available

[Alexander Viro]

On Tue, 9 Oct 2001, Richard Gooch wrote:

> Alexander Viro writes:
> > ... doesn't fix _under_run in try_modload() (see what happens if
> > namelen is 255 and parent is devfs root)
> 
> What underrun?
> How can this be a problem? Before I use pos, I have the following
> check:
>     if (namelen >= STRING_LENGTH) return -ENAMETOOLONG;
> 
> so the maximum value that namelen can be is STRING_LENGTH-1. Thus we
> have:
>     pos = STRING_LENGTH - (STRING_LENGTH - 1) - 1;
> ->  pos = 0;

Certainly.  And
     buf[STRING_LENGTH - namelen - 2] = '/';
assigns '/' to
	buf [ STRING_LENGTH - (STRING_LENGTH-1) - 2 ]
i.e.
	buf[-1]

> Ah, shit. I just checked the rwsem implementation. It seems that once
> we do a down_write() (even if that blocks because someone else has a
> down_read() already), subsequent down_read() calls will block until
> the writer is granted access and then does up_write(). Damn. It would
> have been good for this to be documented somewhere. Those are the
> kinds of traps that should be mentioned in the header file.
>
> OK: is there a variant of rwsem which is "unfair" (i.e. readers can
> starve writers indefinately)?

	IMO it's a wrong approach.  Notice that all these problems
have common reason - you are reusing entries.  There's absolutely
no need to do that.  Separate the logics for "search" and "create",
so that devfs_register() would fail if entry already exists.
Detach it from the tree upon unregister().  And add a simple reference
counter to the damn thing.  Set it to 1 when entry is created. Bump it
when you use it up/drop when you stop.  And drop it when you detach
from the tree.  End of story.  Symlink contents is freed along with
the entry when refcount hits zero.  No semaphores, no new locking
primitives, no wheels to reinvent.

	Now, given the unholy mess in your search_...() functions I
don't envy you - cleaning them up _will_ hurt.  Ditto for auditing
the code for places that would retain a reference to unregistered
entries.  But as far as I can see that's the only realistic way
to handle these problems.

Re: [PATCH] devfs v194 available

[Richard Gooch]

Alexander Viro writes:
> On Tue, 9 Oct 2001, Richard Gooch wrote:
> 
> > Alexander Viro writes:
> > > ... doesn't fix _under_run in try_modload() (see what happens if
> > > namelen is 255 and parent is devfs root)
> > 
> > What underrun?
> > How can this be a problem? Before I use pos, I have the following
> > check:
> >     if (namelen >= STRING_LENGTH) return -ENAMETOOLONG;
> > 
> > so the maximum value that namelen can be is STRING_LENGTH-1. Thus we
> > have:
> >     pos = STRING_LENGTH - (STRING_LENGTH - 1) - 1;
> > ->  pos = 0;
> 
> Certainly.  And
>      buf[STRING_LENGTH - namelen - 2] = '/';
> assigns '/' to
> 	buf [ STRING_LENGTH - (STRING_LENGTH-1) - 2 ]
> i.e.
> 	buf[-1]

Bugger. Didn't look further down. Ah, well. Incomplete bug report :-)

> > Ah, shit. I just checked the rwsem implementation. It seems that once
> > we do a down_write() (even if that blocks because someone else has a
> > down_read() already), subsequent down_read() calls will block until
> > the writer is granted access and then does up_write(). Damn. It would
> > have been good for this to be documented somewhere. Those are the
> > kinds of traps that should be mentioned in the header file.
> >
> > OK: is there a variant of rwsem which is "unfair" (i.e. readers can
> > starve writers indefinately)?
> 
> 	IMO it's a wrong approach.  Notice that all these problems
> have common reason - you are reusing entries.  There's absolutely no
> need to do that.  Separate the logics for "search" and "create", so
> that devfs_register() would fail if entry already exists.  Detach it
> from the tree upon unregister().  And add a simple reference counter
> to the damn thing.  Set it to 1 when entry is created. Bump it when
> you use it up/drop when you stop.  And drop it when you detach from
> the tree.  End of story.  Symlink contents is freed along with the
> entry when refcount hits zero.  No semaphores, no new locking
> primitives, no wheels to reinvent.

This is exactly what I've done in my big re-write.

> 	Now, given the unholy mess in your search_...() functions I
> don't envy you - cleaning them up _will_ hurt.

Yes, it *is* hurting :-( Those mutually recursive functions are the
last bits left to convert/burn-at-the-stake in my re-write. They're
the last bits precisely because they're the most ugly.

> Ditto for auditing the code for places that would retain a reference
> to unregistered entries.  But as far as I can see that's the only
> realistic way to handle these problems.

Yep. My new code is looking *much* cleaner.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca