| Message ID | 20241010174817.1543642-4-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5ced52fa8f0dc23adb067f7b8a009a5ee051efb7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: add skb->sk to more control packets | expand |
From: Eric Dumazet <edumazet@google.com> Date: Thu, 10 Oct 2024 17:48:15 +0000 > 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@google.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
On Thu, Oct 10, 2024 at 1:48 PM Eric Dumazet <edumazet@google.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@google.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 >
On Mon, Oct 14, 2024 at 4:20 PM Brian Vazquez <brianvv@google.com> wrote: > > On Thu, Oct 10, 2024 at 1:48 PM Eric Dumazet <edumazet@google.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@google.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 :)
Thanks a lot for the explanation, it makes sense. Reviewed-by: Brian Vazquez <brianvv@google.com> On Mon, Oct 14, 2024 at 10:23 AM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Oct 14, 2024 at 4:20 PM Brian Vazquez <brianvv@google.com> wrote: > > > > On Thu, Oct 10, 2024 at 1:48 PM Eric Dumazet <edumazet@google.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@google.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 :)
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); + 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,
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@google.com> --- include/net/sock.h | 9 +++++++++ net/core/sock.c | 9 +++------ net/ipv4/tcp_output.c | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-)