linux-kernel - Re: [PATCH v2 2/2] kernel-trace: Less calls for iput() in create_trace

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]

Date:	Sun, 16 Nov 2014 14:31:20 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:	Ingo Molnar <mingo@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org, Coccinelle <cocci@...teme.lip6.fr>
Subject: Re: [PATCH v2 2/2] kernel-trace: Less calls for iput() in
 create_trace_uprobe() after error detection

On Sun, 16 Nov 2014 20:22:22 +0100
SF Markus Elfring <elfring@...rs.sourceforge.net> wrote:

> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Sun, 16 Nov 2014 19:49:39 +0100
> 
> The iput() function was called in three cases by the create_trace_uprobe()
> function during error handling even if the passed variable contained still
> a null pointer. This implementation detail could be improved by the
> introduction of another jump label.

The first patch is fine, and the only reason is to save the few bytes
that the branch check might take. It's in a path that is unlikely to be
hit so it is not a performance issue at all.

This patch is useless. I rather not apply any patch than to create
another jump that skips over the freeing of iput() just because we know
inode is null. That's why we had the if (inode) in the first place.

So Nack on this patch and I'll contemplate applying the first one. I
probably will as it seems rather harmless.

Thanks,

-- Steve


> 
> Suggested-by: Julia Lawall <julia.lawall@...6.fr>
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  kernel/trace/trace_uprobe.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index ec002c0..a0288f2 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -434,19 +434,24 @@ static int create_trace_uprobe(int argc, char **argv)
>  	arg = strchr(argv[1], ':');
>  	if (!arg) {
>  		ret = -EINVAL;
> -		goto fail_address_parse;
> +		goto fail_address_parse2;
>  	}
>  
>  	*arg++ = '\0';
>  	filename = argv[1];
>  	ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>  	if (ret)
> -		goto fail_address_parse;
> +		goto fail_address_parse2;
>  
>  	inode = igrab(path.dentry->d_inode);
>  	path_put(&path);
>  
> -	if (!inode || !S_ISREG(inode->i_mode)) {
> +	if (!inode) {
> +		ret = -EINVAL;
> +		goto fail_address_parse2;
> +	}
> +
> +	if (!S_ISREG(inode->i_mode)) {
>  		ret = -EINVAL;
>  		goto fail_address_parse;
>  	}
> @@ -554,6 +559,7 @@ error:
>  fail_address_parse:
>  	iput(inode);
>  
> +fail_address_parse2:
>  	pr_info("Failed to parse address or file.\n");
>  
>  	return ret;

--
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/