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)
Mon, Jan 27, 4:39 PM
Unknown Object (File)
Mon, Jan 27, 4:33 PM
Unknown Object (File)
Sun, Jan 26, 6:03 PM
Unknown Object (File)
Sat, Jan 25, 5:05 PM
Unknown Object (File)
Wed, Jan 22, 2:58 PM
Unknown Object (File)
Wed, Jan 15, 6:42 PM
Unknown Object (File)
Dec 26 2024, 6:10 AM
Unknown Object (File)
Dec 7 2024, 11:15 PM
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 Skipped
Unit
Tests Skipped
Build Status
Buildable 50500
Build 47391: arc lint + arc unit

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
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
341

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
341

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
341

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

sys/fs/fdescfs/fdesc_vnops.c
341

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
341

Exactly. Then it hits the vn_vget_ino() path.

sys/fs/fdescfs/fdesc_vnops.c
341

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
341

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