netdev - Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

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] [day] [month] [year] [list]

Message-ID: <CAKhg4t+SRCXMiuJGMYNw+=xDMW84NtpmKuGw=4L+dkTqX7E7oQ@mail.gmail.com>
Date: Mon, 7 Aug 2023 20:49:34 +0800
From: Liang Chen <liangchen.linux@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net, 
	john.fastabend@...il.com, ilias.apalodimas@...aro.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

On Thu, Aug 3, 2023 at 11:17 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> From: Liang Chen <liangchen.linux@...il.com>
> Date: Thu, 3 Aug 2023 16:25:49 +0800
>
> > On Thu, Aug 3, 2023 at 1:11 AM Alexander Lobakin
> > <aleksander.lobakin@...el.com> wrote:
> >>
> >> From: Liang Chen <liangchen.linux@...il.com>
> >> Date: Wed,  2 Aug 2023 15:04:54 +0800
> >>
> >>> In the generic XDP processing flow, if an skb with a page pool page
> >>> (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> >>> undergo head expansion and linearization of fragment data. As a result,
> >>> skb->head points to a reallocated buffer without any fragments. At this
> >>> point, the skb will not contain any page pool pages. However, the
> >>> skb->pp_recycle flag is still set to 1, which is inconsistent with the
> >>> actual situation. Although it doesn't seem to cause much real harm at the
> >>
> >> This means it must be handled in the function which replaces the head,
> >> i.e. pskb_expand_head(). Your change only suppresses one symptom of the
> >> issue.
> >>
> >
> > I attempted to do so. But after pskb_expand_head, there may still be
> > skb frags with pp pages left. It is after skb_linearize those frags
> > are removed.
>
> Ah, right.
> Then you need to handle that in __pskb_pull_tail(). Check at the end of
> the function whether the skb still has any frags, and if not, clear
> skb->pp_recycle.
>
> The most correct fix would be to do that in both pskb_expand_head() and
> __pskb_pull_tail(): iterate over the frags and check if any page still
> belongs to a page_pool. Then page_pool_return_skb_page() wouldn't hit
> false-branch after the skb was re-layout.
>

Yeah, I agree. netif_receive_generic_xdp may not be the only place the
flag can get wrong. To make the pp_recycle flag strictly reflect the
state of page pool usages, the check should be put in both functions.
But, since it's merely an indication that the skb is 'page pool
aware,' and considering that the performance impact we observed
doesn't affect the native XDP path, addressing this issue doesn't seem
worthwhile based on the feedback I've received.


Thanks,
Liang

> [...]
>
> Thanks,
> Olek