netdev - Re: [PATCH net-next 3/3] strparser: Generalize strparser

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

Date:   Thu, 19 Oct 2017 13:18:20 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Tom Herbert <tom@...bertland.com>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     Tom Herbert <tom@...ntonium.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Rohit Seth <rohit@...ntonium.net>
Subject: Re: [PATCH net-next 3/3] strparser: Generalize strparser

On 10/19/2017 12:51 PM, Tom Herbert wrote:
> On Thu, Oct 19, 2017 at 10:42 AM, Eric Dumazet <eric.dumazet@...il.com>
> wrote:
> 
>> On Fri, 2017-07-28 at 16:22 -0700, Tom Herbert wrote:
>>> Generalize strparser from more than just being used in conjunction
>>> with read_sock. strparser will also be used in the send path with
>>> zero proxy. The primary change is to create strp_process function
>>> that performs the critical processing on skbs. The documentation
>>> is also updated to reflect the new uses.
>>>
>>> Signed-off-by: Tom Herbert <tom@...ntonium.net>
>>> ---
>>>  Documentation/networking/strparser.txt | 207 +++++++++++++++-------
>>>  include/net/strparser.h                | 119 +++++++------
>>>  net/kcm/kcmproc.c                      |  34 ++--
>>>  net/kcm/kcmsock.c                      |  38 ++--
>>>  net/strparser/strparser.c              | 313
>> ++++++++++++++++++++-------------
>>>  5 files changed, 424 insertions(+), 287 deletions(-)
>>
>> Just found this gem :
>>
>> static void strp_msg_timeout(unsigned long arg)
>> {
>>         struct strparser *strp = (struct strparser *)arg;
>>
>>         /* Message assembly timed out */
>>         STRP_STATS_INCR(strp->stats.msg_timeouts);
>>         strp->cb.lock(strp);
>>         strp->cb.abort_parser(strp, ETIMEDOUT);
>>         strp->cb.unlock(strp);
>> }
>>
>> static void strp_sock_lock(struct strparser *strp)
>> {
>>         lock_sock(strp->sk);
>> }
>>
>> static void strp_sock_unlock(struct strparser *strp)
>> {
>>         release_sock(strp->sk);
>> }
>>
>>
>> A timer runs from BH, and from this interrupt context it is absolutely
>> illegal to call lock_sock() ( and release_sock() )
>>
>> Please fix, thanks !
>>
> 
> Nice catch! I'll fix it.
> 
Can the lock/release be removed and the strp stopped bit can use atomics?

It looks like sk_error_report can be called without sock lock.

Thanks,
John