netdev - Re: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of frames are received

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]

Date:   Mon, 17 May 2021 22:35:44 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...il.com>
To:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of
 frames are received

Hello!

On 5/10/21 1:29 PM, Yoshihiro Shimoda wrote:

>>>     Posting a review of the already commited (over my head) patch. It would have
>>> been appropriate if the patch looked OK but it's not. :-/
>>>
>>>> When a lot of frames were received in the short term, the driver
>>>> caused a stuck of receiving until a new frame was received. For example,
>>>> the following command from other device could cause this issue.
>>>>
>>>>      $ sudo ping -f -l 1000 -c 1000 <this driver's ipaddress>
>>>
>>>     -l is essential here, right?
> 
> Yes.
> 
>>>     Have you tried testing sh_eth sriver like that, BTW?
>>
>>     It's driver! :-)
> 
> I have not tried testing sh_eth driver yet. I'll test it after I got an actual board.

   Now you've got it, let's not rush forth with the fix this time.

>>>> The previous code always cleared the interrupt flag of RX but checks
>>>> the interrupt flags in ravb_poll(). So, ravb_poll() could not call
>>>> ravb_rx() in the next time until a new RX frame was received if
>>>> ravb_rx() returned true. To fix the issue, always calls ravb_rx()
>>>> regardless the interrupt flags condition.
>>>
>>>     That bacially defeats the purpose of IIUC...
>>                                            ^ NAPI,
>>
>>     I was sure I typed NAPI here, yet it got lost in the edits. :-)
> 
> I could not understand "that" (calling ravb_rx() regardless the interrupt
> flags condition) defeats the purpose of NAPI. According to an article on
> the Linux Foundation wiki [1], one of the purpose of NAPI is "Interrupt mitigation".

   Thank you for the pointer, BTW! Would have helped me with enabling NAPI in sh_eth
(and ravb) drivers...

> In poll(), the interrupts are already disabled, and ravb_rx() will check the
> descriptor's status. So, this patch keeps the "Interrupt mitigation" IIUC.

   I think we'll still have the short race window, described in section 5.1
of this doc. So perhaps what we should do is changing the order of the code in
the poll() method, not eliminating the loops totally. Thoughts?
 
> [1]
> https://wiki.linuxfoundation.org/networking/napi
> 
> Best regards,
> Yoshihiro Shimoda

MBR, Sergei