2003-08-28 19:13:12

by Ulrich Drepper

[permalink] [raw]
Subject: ->pid in filesystem code

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I looked briefly through the filesystem code. That's all definitely out
of my league so I don't try to make a final call or change something.
Anyway, this is what I found, the owners of that code should probably
look at it. Filesystems not mentioned are fine. I've ignored uses of
- ->pid in print statements; it's ok, any maybe preferable, there.


cifs:

apparently uses current->pid to keep track of locking. This might
mean that the current implementation is actually getting things very
wrong, at least from the Unix semantics. Locking happens on process
basis. I count 11 uses of ->pid, all suspicious. Using this
filesystem with NPTL seems to be risky in the moment.

coda:

One use in upcall.c. Seems fishy if it is assumed that the code can
be executed by any process. If it is only meant to be used by the
userlevel part of CODA then it should be fine. Might be good to
add a comment, though.

intermezzo:

Wow, don't know where to start. A gazillion uses of ->pid. Some are
print statements but there are others where the value is assigned to
elements of some internal data structures. I think I would strongly
suggest to avoid this filesystem when using NPTL until it is clear
that there are no issues.

lockd:

In clntproc.c the ->pid value is used to generate some kind of token.
Again, the thread can go away and take the PID with it while the
process remains. Don't know whether this is a problem here.

nfs:

Should be ok. Only mentioned in nfsXproc.c where the PID of the
server is returned to the client.

umsdos:

The pid seems to be used for some kind of locking. Might be that
using ->pid is correct here. In that case it needs comments.


There rest seems to be fine. Including ext2/3 which use the ->pid value
for coloring.

- --
- --------------. ,-. 444 Castro Street
Ulrich Drepper \ ,-----------------' \ Mountain View, CA 94041 USA
Red Hat `--' drepper at redhat.com `---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE/TlQm2ijCOnn/RHQRAmyWAKCBC+cPr3ebdoeiqpusTZPn6+3cVwCffBLS
6hWR3C2+8NKck8FxAAlZun8=
=9UyG
-----END PGP SIGNATURE-----


2003-08-29 08:17:54

by Al Viro

[permalink] [raw]
Subject: Re: ->pid in filesystem code

On Thu, Aug 28, 2003 at 12:12:38PM -0700, Ulrich Drepper wrote:
> cifs:
>
> apparently uses current->pid to keep track of locking. This might
> mean that the current implementation is actually getting things very
> wrong, at least from the Unix semantics. Locking happens on process
> basis. I count 11 uses of ->pid, all suspicious. Using this
> filesystem with NPTL seems to be risky in the moment.

s/with NPTL//. I'm fairly certain that fs/cifs went into the tree without
a review and what's more, in this case I have very strong suspicion that
it might have been deliberately obfuscated to scare potential reviewers off.
Whatever the cause might be, the code *is* obfuscated enough to make it very
hard to review and it certainly contains a lot of dubious stuff.


> intermezzo:
>
> Wow, don't know where to start. A gazillion uses of ->pid. Some are
> print statements but there are others where the value is assigned to
> elements of some internal data structures. I think I would strongly
> suggest to avoid this filesystem when using NPTL until it is clear
> that there are no issues.

intermezzo needs a serious rewrite before it will be usable in 2.6. Authors
had promised to do something about it, but so far it hadn't reached the Linus'
tree.

> umsdos:
>
> The pid seems to be used for some kind of locking. Might be that
> using ->pid is correct here. In that case it needs comments.

Doesn't even build. Will need a rewrite or removal - it had been rotting
for a *long* time.