nfs and cd9660: normalize handling of va_rdev

[kib]

Summary

cd9660: va_rdev should be NODEV for non-special inodes

Reported by:    pho


nfsclient: va_rdev should be NODEV for non-special nodes

Server is allowed to fill any value into the rdev attribute, clear it to
satisfy the local requirements.

Reported by:    bakul

[kib]

sys/fs/nfsclient/nfs_clport.c

469 ↗

(On Diff #160523)

Somewhat (un)related: nfscl_loadattrcache() is often called with the vnode only shared locked. This flip of the vnode type and reset of v_op is quite broken then.

[rmacklem]

sys/fs/nfsclient/nfs_clport.c

469 ↗

(On Diff #160523)

To be honest, this code is so old it probably
pre-dates shared vnode locks.

I cannot remember when this actually can
happen (maybe just when there is a newly
created vnode?).

I'll poke at it (and maybe add a check for the
exclusive vnode lock in there to see if it ever
gets triggered).
(But not this week.)

[kib]

sys/fs/nfsclient/nfs_clport.c

469 ↗

(On Diff #160523)

A malignant server might trigger this to corrupt the client state.

[rmacklem]

sys/fs/nfsclient/nfs_clport.c

469 ↗

(On Diff #160523)

Yep. There used to be a "caching mechanism" inside
some network switch that cached Getattr replies.
Unfortunately the code was buggy and sometimes
replied with the reply for the wrong Getattr.
(The guy who told me this was under NDA,
so I never found out which vendor, etc.)
That's where the printf() at line#398 in nfs_clport.c
comes from.

If my hunch is correct, the only valid case
is VNON->whatever. If that is correct, other
cases should probably not "switch" the type.

[kib]

sys/fs/nfsclient/nfs_clport.c

469 ↗

(On Diff #160523)

Then the right action is perhaps to vgone() the vp. Problem is that vgone() requires exclusively locked vnode too.

[des]

I have a simpler solution for NFS: D51996

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

Why not just use VATTR_ISDEV() here?

[kib]

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

I decided to go with this version, instead of trying to patch the server response, because the vnode type and the server reply (cached in attrs/nattrs) might be not synchronized. As an example of that, see the discussion on the left side of the review, for lines around 468 etc.

So I wanted to use the exact data (the vp->v_type) same as used in the currently reverted assert.

[des]

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

Perhaps we should assert that vp->v_type == vap->va_type?

[kib]

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

This would give a broken or malicious NFS server one more way to trick nfs client into problems.

[des]

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

The types have to match though. And look at line 469 below...

[kib]

sys/fs/nfsclient/nfs_clport.c

432 ↗

(On Diff #160523)

For the line 469, see the discussion on the left pane, exactly. This was the motivation for the approach I choose.