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)
Fri, Sep 20, 11:03 PM
Unknown Object (File)
Fri, Sep 20, 2:11 PM
Unknown Object (File)
Fri, Sep 20, 2:01 AM
Unknown Object (File)
Thu, Sep 19, 11:11 AM
Unknown Object (File)
Tue, Sep 17, 3:22 AM
Unknown Object (File)
Mon, Sep 16, 8:15 PM
Unknown Object (File)
Thu, Sep 5, 11:24 PM
Unknown Object (File)
Sun, Sep 1, 6:01 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 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)