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: <CAMzD94Q5iwiOrBOtOMhL27Cus6HJajFW4PSuSvx0DxKtySNiDg@mail.gmail.com>
Date: Mon, 14 Oct 2024 10:34:45 -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

Thanks a lot for the explanation, it makes sense.

Reviewed-by: Brian Vazquez <brianvv@...gle.com>

On Mon, Oct 14, 2024 at 10:23 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> 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 :)