linux-kernel - Re: [PATCH 1/1] ceph: Deletion of unnecessary checks before two function calls

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: <CALFYKtAJG5W9-BAHrqEcC-FfqdJERCqkBADo4yTUyjzb+wCHnQ@mail.gmail.com>
Date:	Mon, 3 Nov 2014 14:35:28 +0400
From:	Ilya Dryomov <ilya.dryomov@...tank.com>
To:	SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:	Sage Weil <sage@...tank.com>,
	Ceph Development <ceph-devel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org, trivial@...nel.org,
	Coccinelle <cocci@...teme.lip6.fr>,
	"Yan, Zheng" <zyan@...hat.com>
Subject: Re: [PATCH 1/1] ceph: Deletion of unnecessary checks before two
 function calls

On Sun, Nov 2, 2014 at 5:20 PM, SF Markus Elfring
<elfring@...rs.sourceforge.net> wrote:
> The functions ceph_put_snap_context() and iput() test whether their argument
> is NULL and then return immediately. Thus the test around the call
> is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  fs/ceph/caps.c       | 3 +--
>  fs/ceph/mds_client.c | 6 ++----
>  fs/ceph/snap.c       | 9 +++------
>  3 files changed, 6 insertions(+), 12 deletions(-)

[CC'ed Zheng]

Applied, but see below.

>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 6d1cd45..7d99fc8 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3136,8 +3136,7 @@ flush_cap_releases:
>  done:
>         mutex_unlock(&session->s_mutex);
>  done_unlocked:
> -       if (inode)
> -               iput(inode);
> +       iput(inode);
>         return;
>
>  bad:
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index bad07c0..3b0ab05 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -523,8 +523,7 @@ void ceph_mdsc_release_request(struct kref *kref)
>         }
>         if (req->r_locked_dir)
>                 ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
> -       if (req->r_target_inode)
> -               iput(req->r_target_inode);
> +       iput(req->r_target_inode);
>         if (req->r_dentry)
>                 dput(req->r_dentry);

dput() also checks for NULL argument, but the check is wrapped into
unlikely(), which is why I presume it wasn't picked up.  It would be
great if you could improve your coccinelle script to handle
{un,}likely() as well.

>         if (req->r_old_dentry)
> @@ -995,8 +994,7 @@ out:
>         session->s_cap_iterator = NULL;
>         spin_unlock(&session->s_cap_lock);
>
> -       if (last_inode)
> -               iput(last_inode);
> +       iput(last_inode);
>         if (old_cap)
>                 ceph_put_cap(session->s_mdsc, old_cap);
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index f01645a..c1cc993 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -365,8 +365,7 @@ static int build_snap_context(struct ceph_snap_realm *realm)
>              realm->ino, realm, snapc, snapc->seq,
>              (unsigned int) snapc->num_snaps);
>
> -       if (realm->cached_context)
> -               ceph_put_snap_context(realm->cached_context);
> +       ceph_put_snap_context(realm->cached_context);
>         realm->cached_context = snapc;
>         return 0;
>
> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
> *realm)

The patch was corrupted, that should have been a single line.  I fixed
it up but you may want to look into your email client settings.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/