On Fri, May 05, 2023 at 10:22:09AM +0200, Christian König wrote:
Am 05.05.23 um 05:03 schrieb ye.xingchen@zte.com.cn:
From: Ye Xingchen ye.xingchen@zte.com.cn
convert the fget() use to fdget().
Well the rational is missing. Why should we do that?
We very definitely should not. The series appears to be pure cargo-culting and it's completely wrong.
There is such thing as unwarranted use of fget(). Under some conditions converting to fdget() is legitimate *and* is an improvement. HOWEVER, those conditions are not met in this case.
Background: references in descriptor table do contribute to struct file refcount. fget() finds the reference by descriptor and returns it, having bumped the refcount. In case when descriptor table is shared, we must do that - otherwise e.g. close() or dup2() from another thread could very well have destroyed the struct file we'd just found. However, if descriptor table is *NOT* shared, there's no need to mess with refcount at all. Provided that * we are not grabbing the reference to keep it (stash into some data structure, etc.); as soon as we return from syscall, the reference in descriptor table is fair game for e.g. close(2). Or exit(2), for that matter. * we remember whether it was shared or not - we can't just recheck that when we are done with the file; after all, descriptor table might have been shared when we looked the file up, but another thread might've died since then and left it not shared anymore. * we do not rip the same reference out of our descriptor table ourselves - not without seriously convoluted precautions. Very few places in the kernel can lead to closing descriptors, so in practice it only becomes a problem when a particularly ugly ioctl decides that it would be neat to close some descriptor(s). Example of such convolutions: binder_deferred_fd_close().
fdget() returns a pair that consists of struct file reference *AND* indication whether we have grabbed a reference. fdput() takes such pair.
Both are inlined, and compiler is smart enough to split the pair into two separate local variables. The underlying primitive actually stashes the "have grabbed the refcount" into the LSB of returned word; see __to_fd() in include/linux/file.h for details. It really generates a decent code and a plenty of places where we want a file by descriptor are just fine with it.
This patch is flat-out broken, since it loses the "have we bumped the refcount" information - the callers do not get it.
It might be possible to massage the calling conventions to enable the conversion to fdget(), but it's not obvious that result would be cleaner and in any case, the patch in question doesn't even try that.