netdev - Re: [PATCH v2 net 1/2] dccp/tcp: Avoid negative sk_forward_alloc by ipv6

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: <CANn89iJ2SpzuL9wyCeDjadogiUfk2C1niJD3RG7tJHA+T1aiJA@mail.gmail.com>
Date:   Thu, 9 Feb 2023 12:20:04 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
        Andrii <tulup@...l.ru>,
        Arnaldo Carvalho de Melo <acme@...driva.com>
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:
>
> 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...

Something like this (this can also be made out-of-line, because this
is not fast path)

Name is probably not well chosen...

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 */