linux-kernel - Re: [PATCH 13/18] tracing: Add array type to function based events

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Feb 2018 20:54:31 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Tom Zanussi <tom.zanussi@...ux.intel.com>,
        linux-rt-users@...r.kernel.org, linux-trace-users@...r.kernel.org,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Clark Williams <williams@...hat.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        kernel-team@....com
Subject: Re: [PATCH 13/18] tracing: Add array type to function based events

On Fri, 9 Feb 2018 10:17:45 +0900
Namhyung Kim <namhyung@...nel.org> wrote:

> > + # echo 1 > events/functions/ip_rcv/enable
> > + # cat trace
> > +    <idle>-0     [003] ..s3   219.813582: __netif_receive_skb_core->ip_rcv(skb=ffff880118195e00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   219.813595: __netif_receive_skb_core->ip_rcv(skb=ffff880118195e00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   220.115053: __netif_receive_skb_core->ip_rcv(skb=ffff880118195c00, perm_addr=b4,b5,2f,ce,18,65)
> > +    <idle>-0     [003] ..s3   220.115293: __netif_receive_skb_core->ip_rcv(skb=ffff880118195c00, perm_addr=b4,b5,2f,ce,18,65)  
> 
> What about adding braces to indicate array type like below?
> 
> ... ip_rcv(skb=ffff880118195c00, perm_addr={b4,b5,2f,ce,18,65})
> 

That's a nice idea, I'll add it.

> > +	case FUNC_STATE_ARRAY:
> >  	case FUNC_STATE_BRACKET:
> > -		WARN_ON(!fevent->last_arg);
> > +		if (WARN_ON(!fevent->last_arg))
> > +			break;
> >  		ret = kstrtoul(token, 0, &val);
> >  		if (ret)
> >  			break;
> > -		val *= fevent->last_arg->size;
> > -		fevent->last_arg->indirect = val ^ INDIRECT_FLAG;
> > -		return FUNC_STATE_INDIRECT;
> > +		if (state == FUNC_STATE_BRACKET) {
> > +			val *= fevent->last_arg->size;
> > +			fevent->last_arg->indirect = val ^ INDIRECT_FLAG;
> > +			return FUNC_STATE_INDIRECT;
> > +		}
> > +		if (val <= 0)
> > +			break;  
> 
> The val is unsigned long type.

I probably should make it a cap it for the array, as arrays that are
too big will simply fail to allocate on the ring buffer.

But it should only check for zero.

> 
> 
> > +		fevent->last_arg->array = val;
> > +		type = kasprintf(GFP_KERNEL, "%s[%d]", fevent->last_arg->type, (unsigned)val);  
> 
> s/%d/%lu/  and no need to cast it.

Sure.

> 
> 
> > +		if (!type)
> > +			break;
> > +		kfree(fevent->last_arg->type);
> > +		fevent->last_arg->type = type;
> > +		/*
> > +		 * arg_offset has already been updated once by size.
> > +		 * This update needs to account for that (hence the "- 1").
> > +		 */
> > +		fevent->arg_offset += fevent->last_arg->size * (fevent->last_arg->array - 1);
> > +		return FUNC_STATE_ARRAY_SIZE;
> > +
> > +	case FUNC_STATE_ARRAY_SIZE:
> > +		if (token[0] != ']')
> > +			break;
> > +		return FUNC_STATE_ARRAY_END;
> >  
> >  	case FUNC_STATE_INDIRECT:
> >  		if (token[0] != ']')
> > @@ -453,6 +485,10 @@ static long long get_arg(struct func_arg *arg, unsigned long val)
> >  
> >  	val = val + (arg->indirect ^ INDIRECT_FLAG);
> >  
> > +	/* Arrays do their own indirect reads */
> > +	if (arg->array)
> > +		return val;
> > +  
> 
> Not sure about this.  After this change it would make 'x64[1] foo' and
> 'x64[1] foo[0]' equivalent, right?

Yeah, I may need to re-think this. I originally had the "array"
use the "indirect" code, but I'm thinking that isn't necessary.

Thanks for the input.

-- Steve