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: <CANn89iLm+VJFdKwsLNoJFuGzA8KGS7b813e38fhiFLk6R9tFUw@mail.gmail.com>
Date: Mon, 14 Oct 2024 16:23:36 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Brian Vazquez <brianvv@...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 Mon, Oct 14, 2024 at 4:20 PM Brian Vazquez <brianvv@...gle.com> wrote:
>
> 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?
>

skb_orphan(skb) does nothing if there is nothing to do.

It is common practice to include it every time we are about to change
skb->destructor.

Otherwise we would have to add a WARN() or something to prevent future leaks.

Better safe than sorry :)