netdev - Re: [PATCH v3 net-next 3/5] net: add skb_set_owner

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: <CAMzD94SwQhKO_-8Xi5axbjb7X+Hb6n99yvQFQkzHUMcyKhRFqg@mail.gmail.com>
Date: Mon, 14 Oct 2024 10:20:28 -0400
From: Brian Vazquez <brianvv@...gle.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Martin KaFai Lau <martin.lau@...nel.org>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, Neal Cardwell <ncardwell@...gle.com>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com
Subject: Re: [PATCH v3 net-next 3/5] net: add skb_set_owner_edemux() helper

On Thu, Oct 10, 2024 at 1:48 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> This can be used to attach a socket to an skb,
> taking a reference on sk->sk_refcnt.
>
> This helper might be a NOP if sk->sk_refcnt is zero.
>
> Use it from tcp_make_synack().
>
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> ---
>  include/net/sock.h    | 9 +++++++++
>  net/core/sock.c       | 9 +++------
>  net/ipv4/tcp_output.c | 2 +-
>  3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 703ec6aef927337f7ca6798ff3c3970529af53f9..e5bb64ad92c769f3edb8c2dc72cafb336837cabb 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1758,6 +1758,15 @@ void sock_efree(struct sk_buff *skb);
>  #ifdef CONFIG_INET
>  void sock_edemux(struct sk_buff *skb);
>  void sock_pfree(struct sk_buff *skb);
> +
> +static inline void skb_set_owner_edemux(struct sk_buff *skb, struct sock *sk)
> +{
> +       skb_orphan(skb);

Is this skb_orphan(skb) needed? IIUC skb_set_owner_w is doing
skb_orphan too? and then calling this helper, but we do need the
skb_orphan is needed when called from the synack.

Can skb_set_owner_w try to orphan an skb twice?


> +       if (refcount_inc_not_zero(&sk->sk_refcnt)) {
> +               skb->sk = sk;
> +               skb->destructor = sock_edemux;
> +       }
> +}
>  #else
>  #define sock_edemux sock_efree
>  #endif
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 083d438d8b6faff60e2e3cf1f982eb306a923cf7..f8c0d4eda888cf190b87fb42e94eef4fb950bf1f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2592,14 +2592,11 @@ void __sock_wfree(struct sk_buff *skb)
>  void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
>  {
>         skb_orphan(skb);
> -       skb->sk = sk;
>  #ifdef CONFIG_INET
> -       if (unlikely(!sk_fullsock(sk))) {
> -               skb->destructor = sock_edemux;
> -               sock_hold(sk);
> -               return;
> -       }
> +       if (unlikely(!sk_fullsock(sk)))
> +               return skb_set_owner_edemux(skb, sk);
>  #endif
> +       skb->sk = sk;
>         skb->destructor = sock_wfree;
>         skb_set_hash_from_sk(skb, sk);
>         /*
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 1251510f0e58da6b6403d2097b498f3e4cb6d255..4cf64ed13609fdcb72b3858ca9e20a1e65bd3d94 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3731,7 +3731,7 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
>
>         switch (synack_type) {
>         case TCP_SYNACK_NORMAL:
> -               skb_set_owner_w(skb, req_to_sk(req));
> +               skb_set_owner_edemux(skb, req_to_sk(req));
>                 break;
>         case TCP_SYNACK_COOKIE:
>                 /* Under synflood, we do not attach skb to a socket,
> --
> 2.47.0.rc1.288.g06298d1525-goog
>