On Fri, May 03, 2024 at 04:41:19PM -0700, Linus Torvalds wrote: > On Fri, 3 May 2024 at 16:23, Kees Cook <keesc...@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
The one the vmgfx isn't really needed (I think at least), because all other drivers that use gem or ttm use the dma_buf export cache in drm/drm_prime.c, which is protected by a bog standard mutex. vmwgfx is unfortunately special in a lot of ways due to somewhat parallel dev history. So there might be an uapi reason why the weak reference is required. I suspect because vmwgfx is reinventing a lot of its own wheels it can't play the same tricks as gem_prime.c, which hooks into a few core drm cleanup/release functions. tldr; drm really has no architectural need for a get_file_unless_doomed, and I certainly don't want to spread it it further than the vmwgfx historical special case that was added in 2013. -Sima > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch