netdev - Re: [PATCH net] gve: Fix an edge case for TSO skb validity check

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: <CA+f9V1PsjukhgLDjWQvbTyhHkOWt7JDY0zPWc_G322oKmasixA@mail.gmail.com>
Date: Thu, 18 Jul 2024 18:51:58 -0700
From: Praveen Kaligineedi <pkaligineedi@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, willemb@...gle.com, shailend@...gle.com, 
	hramamurthy@...gle.com, csully@...gle.com, jfraker@...gle.com, 
	stable@...r.kernel.org, Bailey Forrest <bcf@...gle.com>, 
	Jeroen de Borst <jeroendb@...gle.com>
Subject: Re: [PATCH net] gve: Fix an edge case for TSO skb validity check

On Thu, Jul 18, 2024 at 4:07 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:

> > +                      * segment, then it will count as two descriptors.
> > +                      */
> > +                     if (last_frag_size > GVE_TX_MAX_BUF_SIZE_DQO) {
> > +                             int last_frag_remain = last_frag_size %
> > +                                     GVE_TX_MAX_BUF_SIZE_DQO;
> > +
> > +                             /* If the last frag was evenly divisible by
> > +                              * GVE_TX_MAX_BUF_SIZE_DQO, then it will not be
> > +                              * split in the current segment.
>
> Is this true even if the segment did not start at the start of the frag?
The comment probably is a bit confusing here. The current segment
we are tracking could have a portion in the previous frag. The code
assumed that the portion on the previous frag (if present) mapped to only
one descriptor. However, that portion could have been split across two
descriptors due to the restriction that each descriptor cannot exceed 16KB.
That's the case this fix is trying to address.
I will work on simplifying the logic based on your suggestion below so
that the fix is easier to follow
>
> Overall, it's not trivial to follow. Probably because the goal is to
> count max descriptors per segment, but that is not what is being
> looped over.
>
The comment
> Intuitive (perhaps buggy, a quick sketch), this is what is intended,
> right?
>
> static bool gve_can_send_tso(const struct sk_buff *skb)
> {
>         int frag_size = skb_headlen(skb) - header_len;
>         int gso_size_left;
>         int frag_idx = 0;
>         int num_desc;
>         int desc_len;
>         int off = 0;
>
>         while (off < skb->len) {
>                 gso_size_left = shinfo->gso_size;
>                 num_desc = 0;
>
>                 while (gso_size_left) {
>                         desc_len = min(gso_size_left, frag_size);
>                         gso_size_left -= desc_len;
>                         frag_size -= desc_len;
>                         num_desc++;
>
>                         if (num_desc > max_descs_per_seg)
>                                 return false;
>
>                         if (!frag_size)
>                                 frag_size = skb_frag_size(&shinfo->frags[frag_idx++]);
>                 }
>         }
>
>         return true;
> }
>
> This however loops skb->len / gso_size. While the above modulo
> operation skips many segments that span a frag. Not sure if the more
> intuitive approach could be as performant.
>
> Else, I'll stare some more at the suggested patch to convince myself
> that it is correct and complete..