netdev - Re: [PATCH v2 net 1/2] dccp/tcp: Avoid negative sk_forward_alloc by ipv6_pinfo.pktoptions.> On Thu, Feb 9, 2023 at 2:34 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:

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: <20230209180944.55957-1-kuniyu@amazon.com>
Date:   Thu, 9 Feb 2023 10:09:44 -0800
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <edumazet@...gle.com>
CC:     <acme@...driva.com>, <davem@...emloft.net>, <kuba@...nel.org>,
        <kuni1840@...il.com>, <kuniyu@...zon.com>,
        <netdev@...r.kernel.org>, <pabeni@...hat.com>, <tulup@...l.ru>
Subject: Re: [PATCH v2 net 1/2] dccp/tcp: Avoid negative sk_forward_alloc by ipv6_pinfo.pktoptions.> On Thu, Feb 9, 2023 at 2:34 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:

From:   Eric Dumazet <edumazet@...gle.com>
Date:   Thu, 9 Feb 2023 12:20:04 +0100
> >
> > Eric Dumazet pointed out [0] that when we call skb_set_owner_r()
> > for ipv6_pinfo.pktoptions, sk_rmem_schedule() has not been called,
> > resulting in a negative sk_forward_alloc.
> >
> > Note that in (dccp|tcp)_v6_do_rcv(), we call sk_rmem_schedule()
> > just after skb_clone() instead of after ipv6_opt_accepted().  This is
> > because tcp_send_synack() can make sk_forward_alloc negative before
> > ipv6_opt_accepted() in the crossed SYN-ACK or self-connect() cases.
> >
> > [0]: https://lore.kernel.org/netdev/CANn89iK9oc20Jdi_41jb9URdF210r7d1Y-+uypbMSbOfY6jqrg@mail.gmail.com/
> >
> > Fixes: 323fbd0edf3f ("net: dccp: Add handling of IPV6_PKTOPTIONS to dccp_v6_do_rcv()")
> > Fixes: 3df80d9320bc ("[DCCP]: Introduce DCCPv6")
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> 
> Thanks, but I suggest we add a helper to avoid the duplication...

This is much cleaner, thank you!

> 
> Something like this (this can also be made out-of-line, because this
> is not fast path)
> 
> Name is probably not well chosen...

or skb_clone_and_charge(skb, sk), skb_clone_and_charge_r(skb, sk), or
skb_clone_and_set_owner_r(skb, sk) ?


> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index dcd72e6285b23006051d651630bdd966741cbb01..f5a97aed14345c403b25339fcb86d99bc51233a7
> 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2977,4 +2977,19 @@ static inline bool sk_is_readable(struct sock *sk)
>                 return sk->sk_prot->sock_is_readable(sk);
>         return false;
>  }
> +
> +static inline struct sk_buff *
> +sk_clone_and_charge_skb(struct sock *sk, struct sk_buff *skb)
> +{
> +       skb = skb_clone(skb, sk_gfp_mask(sk, GFP_ATOMIC));
> +       if (skb) {
> +               if (sk_rmem_schedule(sk, skb, skb->truesize)) {
> +                       skb_set_owner_r(skb, sk);
> +                       return skb;
> +               }
> +               __kfree_skb(skb);
> +       }
> +       return NULL;
> +}
> +
>  #endif /* _SOCK_H */