On Mon, Dec 19, 2022 at 4:47 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> Anand hit a BUG() when pulling off headers on egress to a SW tunnel.
> We get to skb_checksum_help() with an invalid checksum offset
> (commit d7ea0d9df2a6 ("net: remove two BUG() from skb_checksum_help()")
> converted those BUGs to WARN_ONs()).
> He points out oddness in how skb_postpull_rcsum() gets used.
> Indeed looks like we should pull before "postpull", otherwise
> the CHECKSUM_PARTIAL fixup from skb_postpull_rcsum() will not
> be able to do its job:
>
> if (skb->ip_summed == CHECKSUM_PARTIAL &&
> skb_checksum_start_offset(skb) < 0)
> skb->ip_summed = CHECKSUM_NONE;
>
> Reported-by: Anand Parthasarathy <anpartha@xxxxxxxx>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> ---
> CC: daniel@xxxxxxxxxxxxx
> CC: martin.lau@xxxxxxxxx
> CC: song@xxxxxxxxxx
> CC: john.fastabend@xxxxxxxxx
> CC: sdf@xxxxxxxxxx
> CC: bpf@xxxxxxxxxxxxxxx
> ---
> net/core/filter.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 929358677183..43cc1fe58a2c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3180,15 +3180,18 @@ static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
>
> static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
> {
> + void *old_data;
> +
> /* skb_ensure_writable() is not needed here, as we're
> * already working on an uncloned skb.
> */
> if (unlikely(!pskb_may_pull(skb, off + len)))
> return -ENOMEM;
>
> - skb_postpull_rcsum(skb, skb->data + off, len);
> - memmove(skb->data + len, skb->data, off);
> + old_data = skb->data;
> __skb_pull(skb, len);
[..]
> + skb_postpull_rcsum(skb, old_data + off, len);
Are you sure about the 'old_data + off' part here (for
CHECKSUM_COMPLETE)? Shouldn't it be old_data?
I'm assuming we need to negate the old parts that we've pulled?
Maybe safer/more correct to do the following?
skb_pull_rcsum(skb, off);
memmove(skb->data, skb->data-off, off);
> + memmove(skb->data, old_data, off);
>
> return 0;
> }
> --
> 2.38.1
>
