nfs and cd9660: normalize handling of va_rdev
ClosedPublic
Actions

Authored by kib on Aug 18 2025, 2:54 AM.

Details

Reviewers

rmacklem
des

Commits

rG6acc5ed5f9c3: nfsclient: va_rdev should be NODEV for non-special nodes
rGfba63c09ed0f: cd9660: va_rdev should be NODEV for non-special inodes

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Aug 18 2025, 2:54 AM

Herald added a subscriber: imp. · View Herald TranscriptAug 18 2025, 2:54 AM

kib requested review of this revision.Aug 18 2025, 2:54 AM

kib added inline comments.

sys/fs/nfsclient/nfs_clport.c
469	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 accepted this revision.Aug 18 2025, 3:23 AM

This revision is now accepted and ready to land.Aug 18 2025, 3:23 AM

rmacklem added inline comments.Aug 18 2025, 3:28 AM

sys/fs/nfsclient/nfs_clport.c
469	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 added inline comments.Aug 18 2025, 3:59 AM

sys/fs/nfsclient/nfs_clport.c
469	A malignant server might trigger this to corrupt the client state.

rmacklem added inline comments.Aug 18 2025, 4:21 AM

sys/fs/nfsclient/nfs_clport.c
469	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 added inline comments.Aug 18 2025, 8:09 AM

sys/fs/nfsclient/nfs_clport.c
469	Then the right action is perhaps to vgone() the vp. Problem is that vgone() requires exclusively locked vnode too.

I have a simpler solution for NFS: D51996

sys/fs/nfsclient/nfs_clport.c
432	Why not just use `VATTR_ISDEV()` here?

kib marked an inline comment as done.Aug 18 2025, 10:36 AM

kib added inline comments.

sys/fs/nfsclient/nfs_clport.c
432	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.