linux-kernel - Re: [PATCH] fs: eventfd: fix obsolete comment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 7 Dec 2019 09:01:39 -0800
From:   Matthew Wilcox <willy@...radead.org>
To:     linmiaohe <linmiaohe@...wei.com>
Cc:     Davide Libenzi <davidel@...ilserver.org>, viro@...iv.linux.org.uk,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: eventfd: fix obsolete comment

On Sat, Dec 07, 2019 at 03:45:33PM +0800, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@...wei.com>
> 
> since commit 36a7411724b1 ("eventfd_ctx_fdget(): use fdget() instead of
> fget()"), this comment become outdated and looks confusing. Fix it with
> the correct function name.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
> ---
>  fs/eventfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/eventfd.c b/fs/eventfd.c
> index 8aa0ea8c55e8..0b8466b12932 100644
> --- a/fs/eventfd.c
> +++ b/fs/eventfd.c
> @@ -352,7 +352,7 @@ EXPORT_SYMBOL_GPL(eventfd_fget);
>   * Returns a pointer to the internal eventfd context, otherwise the error
>   * pointers returned by the following functions:
>   *
> - * eventfd_fget
> + * fdget

But this is wrong.  The error pointer is returned from eventfd_ctx_fileget(),
not from fdget.

Looking at the three callers of eventfd_ctx_fileget(), I think it would
make sense to do this:

diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 997cb5d0a657..c35b614e3770 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -126,11 +126,6 @@ int vfio_virqfd_enable(void *opaque,
 	INIT_WORK(&virqfd->inject, virqfd_inject);
 
 	irqfd = fdget(fd);
-	if (!irqfd.file) {
-		ret = -EBADF;
-		goto err_fd;
-	}
-
 	ctx = eventfd_ctx_fileget(irqfd.file);
 	if (IS_ERR(ctx)) {
 		ret = PTR_ERR(ctx);
diff --git a/fs/eventfd.c b/fs/eventfd.c
index 8aa0ea8c55e8..d389ffd1dc07 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -349,17 +349,13 @@ EXPORT_SYMBOL_GPL(eventfd_fget);
  * eventfd_ctx_fdget - Acquires a reference to the internal eventfd context.
  * @fd: [in] Eventfd file descriptor.
  *
- * Returns a pointer to the internal eventfd context, otherwise the error
- * pointers returned by the following functions:
- *
- * eventfd_fget
+ * Returns a pointer to the internal eventfd context, or an error pointer;
+ * see eventfd_ctx_fileget().
  */
 struct eventfd_ctx *eventfd_ctx_fdget(int fd)
 {
 	struct eventfd_ctx *ctx;
 	struct fd f = fdget(fd);
-	if (!f.file)
-		return ERR_PTR(-EBADF);
 	ctx = eventfd_ctx_fileget(f.file);
 	fdput(f);
 	return ctx;
@@ -368,17 +364,18 @@ EXPORT_SYMBOL_GPL(eventfd_ctx_fdget);
 
 /**
  * eventfd_ctx_fileget - Acquires a reference to the internal eventfd context.
- * @file: [in] Eventfd file pointer.
- *
- * Returns a pointer to the internal eventfd context, otherwise the error
- * pointer:
+ * @file: Eventfd file pointer.
  *
- * -EINVAL   : The @fd file descriptor is not an eventfd file.
+ * Return: A pointer to the internal eventfd context, or an error pointer:
+ * * -EBADF  - The @file is NULL.
+ * * -EINVAL - The @file is not an eventfd file.
  */
 struct eventfd_ctx *eventfd_ctx_fileget(struct file *file)
 {
 	struct eventfd_ctx *ctx;
 
+	if (!file)
+		return ERR_PTR(-EBADF);
 	if (file->f_op != &eventfd_fops)
 		return ERR_PTR(-EINVAL);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 01f3f8b665e9..74b45bc439d8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4676,11 +4676,6 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
 	INIT_WORK(&event->remove, memcg_event_remove);
 
 	efile = fdget(efd);
-	if (!efile.file) {
-		ret = -EBADF;
-		goto out_kfree;
-	}
-
 	event->eventfd = eventfd_ctx_fileget(efile.file);
 	if (IS_ERR(event->eventfd)) {
 		ret = PTR_ERR(event->eventfd);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 67b6fc153e9c..814b99c33d44 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -306,11 +306,6 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 	seqcount_init(&irqfd->irq_entry_sc);
 
 	f = fdget(args->fd);
-	if (!f.file) {
-		ret = -EBADF;
-		goto out;
-	}
-
 	eventfd = eventfd_ctx_fileget(f.file);
 	if (IS_ERR(eventfd)) {
 		ret = PTR_ERR(eventfd);

(not even compile tested)