linux-kernel - Re: [PATCH v3] string.h: Add str_has

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: <20181222092818.GA7610@danjae.aot.lge.com>
Date:   Sat, 22 Dec 2018 18:28:18 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Joe Perches <joe@...ches.com>, LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Tom Zanussi <zanussi@...nel.org>, kernel-team@....com
Subject: Re: [PATCH v3] string.h: Add str_has_prefix() helper

Hi Steve,

On Fri, Dec 21, 2018 at 06:38:17PM -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 18:25:07 -0500
> Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> > On Fri, 21 Dec 2018 15:19:33 -0800
> > Joe Perches <joe@...ches.com> wrote:
> > 
> > > I believe this should be bool.
> > > 
> > > I don't find a use for non-zero assigned len value in the kernel
> > > for strncmp and I believe the function should simply be:
> > > 
> > > static inline bool str_has_prefix(const char *str, const char prefix[])
> > > {
> > > 	return !strncmp(str, prefix, strlen(prefix));
> > > }  
> > 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 3bb2b3351e35..e4566b9c2553 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -172,8 +172,8 @@ cache_type_store(struct device *dev, struct device_attribute *attr,
> >  		 * it's not worth the risk */
> >  		return -EINVAL;
> >  
> > -	if (strncmp(buf, temp, sizeof(temp) - 1) == 0) {
> > -		buf += sizeof(temp) - 1;
> > +	if ((len = str_has_prefix(buf, temp))) {
> > +		buf += len;
> >  		sdkp->cache_override = 1;
> >  	} else {
> >  		sdkp->cache_override = 0;
> > 
> > And there's more places like this.
> >
> 
> Heck, even the tracing code would like this. From the code that started
> this entire discussion:
> 
> -- Steve
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 9d590138f870..632410c6e6c0 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4342,12 +4342,13 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  	unsigned int i;
>  	int ret = 0;
>  	char *str;
> +	int len;
>  
>  	for (i = 0; i < hist_data->attrs->n_actions; i++) {
>  		str = hist_data->attrs->action_str[i];
>  
> -		if (str_has_prefix(str, "onmatch(")) {
> -			char *action_str = str + sizeof("onmatch(") - 1;
> +		if ((len = str_has_prefix(str, "onmatch("))) {
> +			char *action_str = str + len;

IMHO, returning (match) length might confuse people that it might
support partial match and returns the length actually matched rather
than full match.  If I knew it always returns the length of prefix (or
0) why it matters?  Using strlen() in the next line will have same
effect of compiler optimization for the constant strings, no?

		if (str_has_prefix(str, "onmatch(")) {
			char *action_str = str + strlen("onmatch(");

Thanks,
Namhyung


>  
>  			data = onmatch_parse(tr, action_str);
>  			if (IS_ERR(data)) {
> @@ -4355,8 +4356,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  				break;
>  			}
>  			data->fn = action_trace;
> -		} else if (str_has_prefix(str, "onmax(")) {
> -			char *action_str = str + sizeof("onmax(") - 1;
> +		} else if ((len = str_has_prefix(str, "onmax("))) {
> +			char *action_str = str + len;
>  
>  			data = onmax_parse(action_str);
>  			if (IS_ERR(data)) {