netdev - Re: [PATCH v3] net: Add trace events for all receive exit points

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

Message-ID: <afee1f55-e897-704d-83e6-3552d1a6019d@versatic.net>
Date:   Mon, 19 Nov 2018 12:47:31 -0500
From:   Geneviève Bastien <gbastien@...satic.net>
To:     David Miller <davem@...emloft.net>, mathieu.desnoyers@...icios.com
Cc:     netdev@...r.kernel.org, rostedt@...dmis.org, mingo@...hat.com
Subject: Re: [PATCH v3] net: Add trace events for all receive exit points


On 2018-11-19 10:27 a.m., Geneviève Bastien wrote:
> On 2018-11-18 1:19 a.m., David Miller wrote:
>> From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> Date: Sat, 17 Nov 2018 13:27:29 -0500 (EST)
>>
>>> I see two possible solutions:
>>>
>>> 1) Remove the "skb" argument from the sbk_exit tracepoints completely.
>>> Anyway, I think it's not really needed for analysis purposes because
>>> we can link the "entry" with the associated "exit" using the thread ID
>>> executing those tracepoints.  (Genevi�ve, would that work for your
>>> analyses ?)
>>>
>>> 2) Move the skb_exit tracepoints before freeing the skb pointer. My
>>> concern here is that the instrumentation may become much uglier than
>>> the currently proposed patch. (I have not looked at the specifics
>>> though, so I may be wrong.)
>>>
>>> Do you have a preference between those two approaches, or perhaps you
>>> have an alternative solution in mind ?
>> I wonder how other situations handle this.
>>
>> About #2, if you put the tracepoint beforehand you can't log the
>> 'ret' value.  So at least in that regard I prefer #1.
>>
>> If tracepoints generally handle this by matching up the thread
>> ID, then definitely that's how we should do it here too instead
>> of trying to use the SKB pointer for this purpose.
> I would go for #1 too, the "skb" is not used to match entry/exit, it is more the context in which they appear (thread, softirq, etc). And I did indeed get seg faults on my first attempt when I tried to use the existing templates.
>
> There's just the list tracepoint that would now log nothing, so there's no point looping through the list.
>
After further investigation, the 'netif_receive_skb_list_exit' tracepoint poses a problem: There is no return value in that function and the TRACE_EVENT infrastructure does not support tracepoints without payload, afaik.

Here's a few possible solutions:

1) Add a unique trace_netif_receive_skb_list_exit tracepoint with a 'fake' return value of 0, so it can use the same template as the others. Easiest, but looks hackish.

2) Add tracepoints in the callees in the various locations where individual skbs get removed from the list. But that's quite a few code paths to cover, and we may miss a few.

3) Simply ignore the exit of the list code path. Dependency analyses will miss this case though.

4) Replace the proposed exit tracepoints by others after the delivery of packets, ie in the 'netif_receive_skb_internal' and 'netif_receive_skb_list_internal' functions, again at the risk of missing code paths.

5) None of the above (and fallback to using kretprobes with the tracers)


Any one of these approaches sound more appealing?

Thanks,

Geneviève