linux-kernel - Re: [PATCH] FS: nfs: removed goto statement

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20220519234907.GA12990@dev-dsk-fllinden-2c-d7720709.us-west-2.amazon.com>
Date:   Thu, 19 May 2022 23:49:08 +0000
From:   Frank van der Linden <fllinden@...zon.com>
To:     Javier Abrego <javier.abrego.lorente@...il.com>
CC:     <trond.myklebust@...merspace.com>, <anna@...nel.org>,
        <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] FS: nfs: removed goto statement

On Fri, May 20, 2022 at 12:51:15AM +0200, Javier Abrego wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> In this function goto can be replaced. Avoiding goto will improve the
> readability
> 
> Signed-off-by: Javier Abrego<javier.abrego.lorente@...il.com>
> ---
>  fs/nfs/nfs42xattr.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
> index e7b34f7e0..2fc806454 100644
> --- a/fs/nfs/nfs42xattr.c
> +++ b/fs/nfs/nfs42xattr.c
> @@ -743,25 +743,19 @@ void nfs4_xattr_cache_set_list(struct inode *inode, const char *buf,
>         struct nfs4_xattr_entry *entry;
> 
>         cache = nfs4_xattr_get_cache(inode, 1);
> -       if (cache == NULL)
> -               return;
> -
> -       entry = nfs4_xattr_alloc_entry(NULL, buf, NULL, buflen);
> -       if (entry == NULL)
> -               goto out;
> -
> -       /*
> -        * This is just there to be able to get to bucket->cache,
> -        * which is obviously the same for all buckets, so just
> -        * use bucket 0.
> -        */
> -       entry->bucket = &cache->buckets[0];
> -
> -       if (!nfs4_xattr_set_listcache(cache, entry))
> -               kref_put(&entry->ref, nfs4_xattr_free_entry_cb);
> -
> -out:
> -       kref_put(&cache->ref, nfs4_xattr_free_cache_cb);
> +       if (cache == NULL) {
> +               kref_put(&cache->ref, nfs4_xattr_free_cache_cb);
> +       } else {
> +              /*
> +               * This is just there to be able to get to bucket->cache,
> +               * which is obviously the same for all buckets, so just
> +               * use bucket 0.
> +               */
> +               entry->bucket = &cache->buckets[0];
> +
> +               if (!nfs4_xattr_set_listcache(cache, entry))
> +                       kref_put(&entry->ref, nfs4_xattr_free_entry_cb);
> +       }
>  }
> 
>  /*

Hm, I'm not sure what your intention was here, but this patch is wrong
in several ways. It references 'cache' when it's NULL. It removes the
allocation of 'entry' altogether, and then references an uninitialized
variable. Which, surely, gcc would have warned about.

I mean, in the original code, you could replace

if (entry == NULL)
	goto out;

with

if (entry != NULL) {
	...
}

..and remove the out label. Not sure if that would make things massively
more readable.

- Frank