diff mbox series

dma-buf/sync_file: Use fdget()

Message ID 202305051103396748797@zte.com.cn
State New
Headers
Series dma-buf/sync_file: Use fdget() |

Commit Message

ye.xingchen@zte.com.cn May 5, 2023, 3:03 a.m. UTC 
  From: Ye Xingchen <ye.xingchen@zte.com.cn>

convert the fget() use to fdget().

Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
---
 drivers/dma-buf/sync_file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Christian König May 5, 2023, 8:22 a.m. UTC | #1 
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?

Christian.

>
> Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> ---
>   drivers/dma-buf/sync_file.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index af57799c86ce..222b13b1bdb8 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -78,18 +78,18 @@ EXPORT_SYMBOL(sync_file_create);
>
>   static struct sync_file *sync_file_fdget(int fd)
>   {
> -	struct file *file = fget(fd);
> +	struct struct fd f = fdget(fd);
>
> -	if (!file)
> +	if (!f.file)
>   		return NULL;
>
> -	if (file->f_op != &sync_file_fops)
> +	if (f.file->f_op != &sync_file_fops)
>   		goto err;
>
> -	return file->private_data;
> +	return f.file->private_data;
>
>   err:
> -	fput(file);
> +	fdput(f);
>   	return NULL;
>   }
>
Al Viro May 11, 2023, 5:43 a.m. UTC | #2 
On Fri, May 05, 2023 at 11:03:39AM +0800, ye.xingchen@zte.com.cn wrote:
> From: Ye Xingchen <ye.xingchen@zte.com.cn>
> 
> convert the fget() use to fdget().

NAK.
Al Viro May 11, 2023, 6:40 a.m. UTC | #3 
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.
diff mbox series

Patch

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index af57799c86ce..222b13b1bdb8 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -78,18 +78,18 @@  EXPORT_SYMBOL(sync_file_create);

 static struct sync_file *sync_file_fdget(int fd)
 {
-	struct file *file = fget(fd);
+	struct struct fd f = fdget(fd);

-	if (!file)
+	if (!f.file)
 		return NULL;

-	if (file->f_op != &sync_file_fops)
+	if (f.file->f_op != &sync_file_fops)
 		goto err;

-	return file->private_data;
+	return f.file->private_data;

 err:
-	fput(file);
+	fdput(f);
 	return NULL;
 }