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: <6699a042ebdc5_3a5334294df@willemb.c.googlers.com.notmuch>
Date: Thu, 18 Jul 2024 19:07:46 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Praveen Kaligineedi <pkaligineedi@...gle.com>, 
 netdev@...r.kernel.org
Cc: 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>, 
 Praveen Kaligineedi <pkaligineedi@...gle.com>, 
 Jeroen de Borst <jeroendb@...gle.com>
Subject: Re: [PATCH net] gve: Fix an edge case for TSO skb validity check

Praveen Kaligineedi wrote:
> From: Bailey Forrest <bcf@...gle.com>
> 
> The NIC requires each TSO segment to not span more than 10
> descriptors. gve_can_send_tso() performs this check. However,
> the current code misses an edge case when a TSO skb has a large
> frag that needs to be split into multiple descriptors

because each descriptor may not exceed 16KB (GVE_TX_MAX_BUF_SIZE_DQO)

>, causing
> the 10 descriptor limit per TSO-segment to be exceeded. This
> change fixes the edge case.
> 
> Fixes: a57e5de476be ("gve: DQO: Add TX path")
> Signed-off-by: Praveen Kaligineedi <pkaligineedi@...gle.com>
> Signed-off-by: Bailey Forrest <bcf@...gle.com>
> Reviewed-by: Jeroen de Borst <jeroendb@...gle.com>
> ---
>  drivers/net/ethernet/google/gve/gve_tx_dqo.c | 22 +++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index 0b3cca3fc792..dc39dc481f21 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> @@ -866,22 +866,42 @@ static bool gve_can_send_tso(const struct sk_buff *skb)
>  	const int header_len = skb_tcp_all_headers(skb);
>  	const int gso_size = shinfo->gso_size;
>  	int cur_seg_num_bufs;
> +	int last_frag_size;

nit: last_frag can be interpreted as frags[nr_frags - 1], perhaps prev_frag.

>  	int cur_seg_size;
>  	int i;
>  
>  	cur_seg_size = skb_headlen(skb) - header_len;
> +	last_frag_size = skb_headlen(skb);
>  	cur_seg_num_bufs = cur_seg_size > 0;
>  
>  	for (i = 0; i < shinfo->nr_frags; i++) {
>  		if (cur_seg_size >= gso_size) {
>  			cur_seg_size %= gso_size;
>  			cur_seg_num_bufs = cur_seg_size > 0;
> +
> +			/* If the last buffer is split in the middle of a TSO

s/buffer/frag?

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

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.

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

> +                              */


> +				 */
> +				if (last_frag_remain &&
> +				    cur_seg_size > last_frag_remain) {
> +					cur_seg_num_bufs++;
> +				}
> +			}
>  		}
>  
>  		if (unlikely(++cur_seg_num_bufs > max_bufs_per_seg))
>  			return false;
>  
> -		cur_seg_size += skb_frag_size(&shinfo->frags[i]);
> +		last_frag_size = skb_frag_size(&shinfo->frags[i]);
> +		cur_seg_size += last_frag_size;
>  	}
>  
>  	return true;
> -- 
> 2.45.2.993.g49e7a77208-goog
>