Re: [PATCH net-next 8/9] qlcnic: use pskb_may_pull() to pull excess TX frags into the head

[netdev-bot+sashiko@xxxxxxxxxx]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In qlcnic_xmit_frame()
  (drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c:681-688), changing…

--

qlcnic: use pskb_may_pull() to pull excess TX frags into the head

This patch changes qlcnic_xmit_frame() so that it no longer calls
__pskb_pull_tail(skb, delta) directly when a non-GSO skb has more frags
than QLCNIC_MAX_FRAGS_PER_TX. It calls pskb_may_pull(skb,
skb_headlen(skb) + delta) instead. The commit message presents this as
an equivalent API swap.

> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c 
> b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> index 761ef3bc8193a..4a81d499e51c0 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> @@ -682,7 +682,7 @@ netdev_tx_t qlcnic_xmit_frame(struct sk_buff *skb, struct 
> net_device *netdev)
>               for (i = 0; i < (frag_count - QLCNIC_MAX_FRAGS_PER_TX); i++)
>                       delta += skb_frag_size(&skb_shinfo(skb)->frags[i]);
>  
> -             if (!__pskb_pull_tail(skb, delta))
> +             if (!pskb_may_pull(skb, skb_headlen(skb) + delta))
>                       goto drop_packet;
>  
>               frag_count = 1 + skb_shinfo(skb)->nr_frags;

[Severity: Low]
Is this exactly equivalent when delta is 0?

delta is a byte count. It is 0 if all of the leading excess frags of a
non-GSO skb with more than 14 frags are zero-length.

In that case pskb_may_pull_reason() returns early and never calls
__pskb_pull_tail():

include/linux/skbuff.h:pskb_may_pull_reason() {
    ...
        if (likely(len <= skb_headlen(skb)))
                return SKB_NOT_DROPPED_YET;
    ...
}

The old direct call to __pskb_pull_tail(skb, 0) always reached the
pull_pages loop. With eat == 0, every zero-size frag matches
size <= eat, so it is unreferenced and dropped from frags[]:

net/core/skbuff.c:__pskb_pull_tail() {
    ...
pull_pages:
        eat = delta;
        k = 0;
        for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
                int size = skb_frag_size(&skb_shinfo(skb)->frags[i]);

                if (size <= eat) {
                        skb_frag_unref(skb, i);
                        eat -= size;
                } else {
    ...
        }
        skb_shinfo(skb)->nr_frags = k;
    ...
}

With the new call, nr_frags does not change. The recomputed frag_count
stays above QLCNIC_MAX_FRAGS_PER_TX, and nothing checks it again.

After that, qlcnic_map_tx_skb() maps every frag, including the
zero-length ones. qlcnic_set_tx_frags_len() then writes this frag_count
into the first descriptor. The descriptor loop programs more than 14
buffers for a non-TSO packet, which the comment above this block says
the hardware does not support:

        /* 14 frags supported for normal packet and
         * 32 frags supported for TSO packet
         */

This needs an unusual skb layout, and no in-tree producer of it is
known. Even so, could the frag_count be rechecked after the pull, or the
commit message mention this difference?

-- 
Sashiko AI review · 
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-b4-pskb-pull-tail-drivers-v1-0-9512b0fb977b%40toxicpanda.com