Page MenuHomeFreeBSD

fdescfs: Fix a file ref leak
ClosedPublic

Authored by markj on Mar 21 2023, 7:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 3:49 PM
Unknown Object (File)
Oct 14 2024, 9:47 PM
Unknown Object (File)
Oct 3 2024, 5:24 AM
Unknown Object (File)
Oct 2 2024, 5:01 AM
Unknown Object (File)
Oct 1 2024, 10:54 AM
Unknown Object (File)
Oct 1 2024, 8:02 AM
Unknown Object (File)
Sep 30 2024, 4:22 AM
Unknown Object (File)
Sep 30 2024, 2:30 AM
Subscribers

Details

Summary

On github, CTurt points out that vn_vget_ino_gen() may fail without
invoking the callback, in which case the ref on fp is leaked. Moreover,
we cannot safely drop the ref while the dvp is locked.

So:

  • Use a flag variable to indicate whether the ref is dropped.
  • Reorganize things to handle the leak.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Mar 21 2023, 7:15 PM
This revision is now accepted and ready to land.Mar 21 2023, 7:19 PM
kib added inline comments.
sys/fs/fdescfs/fdesc_vnops.c
338–339

I think that the comment should be updated

374

I do not think that error can be zero on this path

markj added inline comments.
sys/fs/fdescfs/fdesc_vnops.c
374

It will be zero when VTOFDESC(dvp)->fd_ix == FD_DESC + fd.

markj marked an inline comment as done.

Adjust a comment.

This revision now requires review to proceed.Mar 21 2023, 8:07 PM
This revision is now accepted and ready to land.Mar 21 2023, 8:43 PM
sys/fs/fdescfs/fdesc_vnops.c
340

I suspect that this check is not enough, for instance the fdescfs root dir can be opened more then once. Perhaps fdescfs vnodes locks should allow recursion.

sys/fs/fdescfs/fdesc_vnops.c
340

But the root dir always has fd_ix == FD_ROOT. In fact, I cannot see how this check is ever true.

sys/fs/fdescfs/fdesc_vnops.c
340

Open /dev/fd with the result file descriptor N, and look up /dev/fd/N.

sys/fs/fdescfs/fdesc_vnops.c
340

Then the root dir has fd_ix = FD_ROOT, which is never equal to FD_DESC + N.

This revision was automatically updated to reflect the committed changes.
sys/fs/fdescfs/fdesc_vnops.c
340

Exactly. Then it hits the vn_vget_ino() path.

sys/fs/fdescfs/fdesc_vnops.c
340

But what exactly is the problem? fdesc_lookup() doesn't lock both vnodes at the same time.

(I did try this experiment, with and without -o nodup.)

sys/fs/fdescfs/fdesc_vnops.c
340

Most likely it was a vnode lock recursion, I just proposed a review. See 60af8a6a7a37d8b02657e45577f019cd13144f19 (I think)