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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGngYiVvuNYC4WPCRfPOfjr98S_BGBNGjPze11AiHY9Pq1eJsA@mail.gmail.com>
Date:   Sun, 31 Jan 2021 10:25:27 -0500
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     Bryan Whitehead <Bryan.Whitehead@...rochip.com>
Cc:     Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Alexey Denisov <rtgbnm@...il.com>,
        Sergej Bauer <sbauer@...ckbox.su>,
        Tim Harvey <tharvey@...eworks.com>,
        Anders Rønningen <anders@...ningen.priv.no>,
        netdev <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v1 2/6] lan743x: support rx multi-buffer packets

On Sun, Jan 31, 2021 at 2:06 AM <Bryan.Whitehead@...rochip.com> wrote:
>
> >  static int lan743x_rx_process_packet(struct lan743x_rx *rx)  {
> It looks like this function no longer processes a packet, but rather only processes a single buffer.
> So perhaps it should be renamed to lan743x_rx_process_buffer, so it is not misleading.

Agreed, will do.

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

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

> ...
> > -               if (!skb) {
> > -                       result = RX_PROCESS_RESULT_PACKET_DROPPED;
> It looks like this return value is no longer used.
> If there is no longer a case where a packet will be dropped
> then maybe this return value should be deleted from the header file.

Agreed, will do.

>
> ...
> >  move_forward:
> > -               /* push tail and head forward */
> > -               rx->last_tail = real_last_index;
> > -               rx->last_head = lan743x_rx_next_index(rx, real_last_index);
> > -       }
> > +       /* push tail and head forward */
> > +       rx->last_tail = rx->last_head;
> > +       rx->last_head = lan743x_rx_next_index(rx, rx->last_head);
> > +       result = RX_PROCESS_RESULT_PACKET_RECEIVED;
>
> Since this function handles one buffer at a time,
>   The return value RX_PROCESS_RESULT_PACKET_RECEIVED is now misleading.
>   Can you change it to RX_PROCESS_RESULT_BUFFER_RECEIVED.

Agreed, will do.

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?

>
>