On Fri, 3 May 2024 at 16:23, Kees Cook keescook@chromium.org wrote:
static bool __must_check get_dma_buf_unless_doomed(struct dma_buf *dmabuf) { return atomic_long_inc_not_zero(&dmabuf->file->f_count) != 0L; }
If we end up adding epi_fget(), we'll have 2 cases of using "atomic_long_inc_not_zero" for f_count. Do we need some kind of blessed helper to live in file.h or something, with appropriate comments?
I wonder if we could try to abstract this out a bit more.
These games with non-ref-counted file structures *feel* a bit like the games we play with non-ref-counted (aka "stashed") 'struct dentry' that got fairly recently cleaned up with path_from_stashed() when both nsfs and pidfs started doing the same thing.
I'm not loving the TTM use of this thing, but at least the locking and logic feels a lot more straightforward (ie the atomic_long_inc_not_zero() here is clealy under the 'prime->mutex' lock
IOW, the tty use looks correct to me, and it has fairly simple locking and is just catching the the race between 'fput()' decrementing the refcount and and 'file->f_op->release()' doing the actual release.
You are right that it's similar to the epoll thing in that sense, it just looks a _lot_ more straightforward to me (and, unlike epoll, doesn't look actively buggy right now).
Could we abstract out this kind of "stashed file pointer" so that we'd have a *common* form for this? Not just the inc_not_zero part, but the locking rule too?
Linus