linux-kernel - Re: [PATCH] sunrpc: Fix trace events to store data in the struct

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: <20150224090915.40d8c7ff@grimm.local.home>
Date:	Tue, 24 Feb 2015 09:09:15 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	James Hogan <james.hogan@...tec.com>
Cc:	Jeff Layton <jlayton@...marydata.com>,
	"J. Bruce Fields" <bfields@...hat.com>,
	<linux-kernel@...r.kernel.org>,
	Trond Myklebust <trond.myklebust@...marydata.com>,
	Ingo Molnar <mingo@...hat.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH] sunrpc: Fix trace events to store data in the struct

On Tue, 24 Feb 2015 11:47:56 +0000
James Hogan <james.hogan@...tec.com> wrote:



>  	TP_printk("xprt=0x%p addr=%pIScp pid=%d flags=%s", __entry->xprt,
> -		(struct sockaddr *)&__entry->xprt->xpt_remote,

There's actually nothing wrong with the above even if xprt is NULL.
It's not dereferencing the structure, it is just getting the address of
what would be dereference.

> -		__entry->rqst ? __entry->rqst->rq_task->pid : 0,
> -		show_svc_xprt_flags(__entry->xprt->xpt_flags))
> +		(struct sockaddr *)&__entry->ss,

The above is meaningless. You just printed the address of the ring
buffer and this will be different (and useless) every time.

> +		__entry->pid,
> +		show_svc_xprt_flags(__entry->flags))
>  );
>  
>  TRACE_EVENT(svc_xprt_dequeue,
> @@ -562,17 +566,21 @@ TRACE_EVENT(svc_handle_xprt,
>  
>  	TP_STRUCT__entry(
>  		__field(struct svc_xprt *, xprt)
> +		__field_struct(struct sockaddr_storage, ss)
> +		__field(unsigned long, flags);
>  		__field(int, len)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->xprt = xprt;
> +		xprt ? memcpy(&__entry->ss, &xprt->xpt_remote, sizeof(__entry->ss)) : memset(&__entry->ss, 0, sizeof(__entry->ss));
> +		__entry->flags = xprt ? xprt->xpt_flags : 0;
>  		__entry->len = len;
>  	),
>  
>  	TP_printk("xprt=0x%p addr=%pIScp len=%d flags=%s", __entry->xprt,
> -		(struct sockaddr *)&__entry->xprt->xpt_remote, __entry->len,
> -		show_svc_xprt_flags(__entry->xprt->xpt_flags))
> +		(struct sockaddr *)&__entry->ss, __entry->len,

Ditto.

Don't use field_struct() unless you really know what you are doing.
This is copying the entire struct into the ring buffer and only using
the address of that struct. Which not only is useless, but wastes a lot
of space in the ring buffer.

-- Steve

> +		show_svc_xprt_flags(__entry->flags))
>  );
>  #endif /* _TRACE_SUNRPC_H */
>  

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