linux-kernel - RE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets

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: <MN2PR11MB3662E5A8E190F8F43F348E72FAB69@MN2PR11MB3662.namprd11.prod.outlook.com>
Date:   Mon, 1 Feb 2021 18:04:44 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <thesven73@...il.com>
CC:     <UNGLinuxDriver@...rochip.com>, <davem@...emloft.net>,
        <kuba@...nel.org>, <andrew@...n.ch>, <rtgbnm@...il.com>,
        <sbauer@...ckbox.su>, <tharvey@...eworks.com>,
        <anders@...ningen.priv.no>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets

Hi Sven, see below

> >
> > If lan743x_rx_init_ring_element fails to allocate an skb, Then
> > lan743x_rx_reuse_ring_element will be called.
> > But that function expects the skb is already allocated and dma mapped.
> > But the dma was unmapped above.
> 
> Good catch. I think you're right, the skb allocation always has to come before
> the unmap. Because if we unmap, and then the skb allocation fails, there is
> no guarantee that we can remap the old skb we've just unmapped (it could
> fail).
> And then we'd end up with a broken driver.
> 
> BUT I actually joined skb alloc and init_ring_element, because of a very
> subtle synchronization bug I was seeing: if someone changes the mtu
> _in_between_ skb alloc and init_ring_element, things will go haywire,
> because the skb and mapping lengths would be different !
> 
> We could fix that by using a spinlock I guess, but synchronization primitives in
> "hot paths" like these are far from ideal... Would be nice if we could avoid
> that.
> 
> Here's an idea: what if we fold "unmap from dma" into init_ring_element()?
> That way, we get the best of both worlds: length cannot change in the
> middle, and the function can always "back out" without touching the ring
> element in case an allocation or mapping fails.
> 
> Pseudo-code:
> 
> init_ring_element() {
>     /* single "sampling" of mtu, so no synchronization required */
>     length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING;
> 
>     skb = alloc(length);
>     if (!skb) return FAIL;
>     dma_ptr = dma_map(skb, length);
>     if (!dma_ptr) {
>         free(skb);
>         return FAIL;
>     }
>     if (buffer_info->dma_ptr)
>         dma_unmap(buffer_info->dma_ptr, buffer_info->buffer_length);
>     buffer_info->skb = skb;
>     buffer_info->dma_ptr = dma_ptr;
>     buffer_info->buffer_length = length;
> 
>     return SUCCESS;
> }
> 
> What do you think?

Yes, I believe this will work.

> 
> >
> > Also if lan743x_rx_init_ring_element fails to allocate an skb.
> > Then control will jump to process_extension and therefor the currently
> > received skb will not be added to the skb list.
> > I assume that would corrupt the packet? Or am I missing something?
> >
> 
> Yes if an skb alloc failure in the middle of a multi-buffer frame, will corrupt
> the packet inside the frame. A chunk will be missing. I had assumed that this
> would be caught by an upper network layer, some checksum would be
> incorrect?
> 
> Are there current networking devices that would send a corrupted packet to
> Linux if there is a corruption on the physical link? Especially if they don't
> support checksumming?
> 
> Maybe my assumption is naive.
> I'll fix this up if you believe that it could be an issue.

Yes the upper layers will catch it and drop it.
But if we already know the packet is corrupted, I think it would be better if we
dropped it here, to avoid unnecessary processing upstream.

...
> 
> RX_PROCESS_RESULT_XXX can now only take two values (RECEIVED and
> NOTHING_TO_DO), so in theory it could be replaced by a bool. But perhaps
> we should keep the current names, because they are clearer to the reader?
> 
I'm ok if you want to change it too a bool.