BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
@ 2019-05-17 21:21 Martin KaFai Lau
  2019-05-17 21:51 ` Eric Dumazet
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Martin KaFai Lau @ 2019-05-17 21:21 UTC (permalink / raw)
  To: bpf, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Joe Stringer

The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
accessed, e.g. type, protocol, mark and priority.
Some new helper, like bpf_sk_storage_get(), also expects
ARG_PTR_TO_SOCKET is a fullsock.

bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
However, the ptr returned from sk_to_full_sk() is not guaranteed
to be a fullsock.  For example, it cannot get a fullsock if sk
is in TCP_TIME_WAIT.

This patch checks for sk_fullsock() before returning. If it is not
a fullsock, sock_gen_put() is called if needed and then returns NULL.

Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
Cc: Joe Stringer <joe@isovalent.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 net/core/filter.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 55bfc941d17a..85def5a20aaf 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
 	struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
 					   ifindex, proto, netns_id, flags);
 
-	if (sk)
+	if (sk) {
 		sk = sk_to_full_sk(sk);
+		if (!sk_fullsock(sk)) {
+			if (!sock_flag(sk, SOCK_RCU_FREE))
+				sock_gen_put(sk);
+			return NULL;
+		}
+	}
 
 	return sk;
 }
@@ -5369,8 +5375,14 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
 	struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
 					 flags);
 
-	if (sk)
+	if (sk) {
 		sk = sk_to_full_sk(sk);
+		if (!sk_fullsock(sk)) {
+			if (!sock_flag(sk, SOCK_RCU_FREE))
+				sock_gen_put(sk);
+			return NULL;
+		}
+	}
 
 	return sk;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-17 21:21 [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup() Martin KaFai Lau
@ 2019-05-17 21:51 ` Eric Dumazet
  2019-05-17 22:01   ` Martin Lau
  2019-05-20 18:57 ` Joe Stringer
  2019-05-21 14:48 ` Daniel Borkmann
  2 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2019-05-17 21:51 UTC (permalink / raw)
  To: Martin KaFai Lau, bpf, netdev
  Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Joe Stringer



On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> accessed, e.g. type, protocol, mark and priority.
> Some new helper, like bpf_sk_storage_get(), also expects
> ARG_PTR_TO_SOCKET is a fullsock.
> 
> bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> However, the ptr returned from sk_to_full_sk() is not guaranteed
> to be a fullsock.  For example, it cannot get a fullsock if sk
> is in TCP_TIME_WAIT.
> 
> This patch checks for sk_fullsock() before returning. If it is not
> a fullsock, sock_gen_put() is called if needed and then returns NULL.
> 
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Cc: Joe Stringer <joe@isovalent.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  net/core/filter.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 55bfc941d17a..85def5a20aaf 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  	struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
>  					   ifindex, proto, netns_id, flags);
>  
> -	if (sk)
> +	if (sk) {
>  		sk = sk_to_full_sk(sk);
> +		if (!sk_fullsock(sk)) {
> +			if (!sock_flag(sk, SOCK_RCU_FREE))
> +				sock_gen_put(sk);

This looks a bit convoluted/weird.

What about telling/asking __bpf_skc_lookup() to not return a non fullsock instead ?

> +			return NULL;
> +		}
> +	}
>  
>  	return sk;
>  }
> @@ -5369,8 +5375,14 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
>  	struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
>  					 flags);
>  
> -	if (sk)
> +	if (sk) {
>  		sk = sk_to_full_sk(sk);
> +		if (!sk_fullsock(sk)) {
> +			if (!sock_flag(sk, SOCK_RCU_FREE))
> +				sock_gen_put(sk);
> +			return NULL;
> +		}
> +	}
>  
>  	return sk;
>  }
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-17 21:51 ` Eric Dumazet
@ 2019-05-17 22:01   ` Martin Lau
       [not found]     ` <CADa=RyxisbcVeXL7yq6o02XOgWd87QCzq-6zDXRnm9RoD2WM=A@mail.gmail.com>
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Lau @ 2019-05-17 22:01 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team,
	Joe Stringer

On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> 
> 
> On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > accessed, e.g. type, protocol, mark and priority.
> > Some new helper, like bpf_sk_storage_get(), also expects
> > ARG_PTR_TO_SOCKET is a fullsock.
> > 
> > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > to be a fullsock.  For example, it cannot get a fullsock if sk
> > is in TCP_TIME_WAIT.
> > 
> > This patch checks for sk_fullsock() before returning. If it is not
> > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > 
> > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > Cc: Joe Stringer <joe@isovalent.com>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> >  net/core/filter.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 55bfc941d17a..85def5a20aaf 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  	struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> >  					   ifindex, proto, netns_id, flags);
> >  
> > -	if (sk)
> > +	if (sk) {
> >  		sk = sk_to_full_sk(sk);
> > +		if (!sk_fullsock(sk)) {
> > +			if (!sock_flag(sk, SOCK_RCU_FREE))
> > +				sock_gen_put(sk);
> 
> This looks a bit convoluted/weird.
> 
> What about telling/asking __bpf_skc_lookup() to not return a non fullsock instead ?
It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
can return non fullsock.

> 
> > +			return NULL;
> > +		}
> > +	}
> >  
> >  	return sk;
> >  }
> > @@ -5369,8 +5375,14 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> >  	struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
> >  					 flags);
> >  
> > -	if (sk)
> > +	if (sk) {
> >  		sk = sk_to_full_sk(sk);
> > +		if (!sk_fullsock(sk)) {
> > +			if (!sock_flag(sk, SOCK_RCU_FREE))
> > +				sock_gen_put(sk);
> > +			return NULL;
> > +		}
> > +	}
> >  
> >  	return sk;
> >  }
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
       [not found]     ` <CADa=RyxisbcVeXL7yq6o02XOgWd87QCzq-6zDXRnm9RoD2WM=A@mail.gmail.com>
@ 2019-05-18 19:05       ` Martin Lau
  2019-05-19  1:52         ` Joe Stringer
  0 siblings, 1 reply; 10+ messages in thread
From: Martin Lau @ 2019-05-18 19:05 UTC (permalink / raw)
  To: Joe Stringer
  Cc: Eric Dumazet, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
	Kernel Team

On Sat, May 18, 2019 at 08:38:46AM -1000, Joe Stringer wrote:
> On Fri, May 17, 2019, 12:02 Martin Lau <kafai@fb.com> wrote:
> 
> > On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> > >
> > >
> > > On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > > > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > > > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > > > accessed, e.g. type, protocol, mark and priority.
> > > > Some new helper, like bpf_sk_storage_get(), also expects
> > > > ARG_PTR_TO_SOCKET is a fullsock.
> > > >
> > > > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > > > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > > > to be a fullsock.  For example, it cannot get a fullsock if sk
> > > > is in TCP_TIME_WAIT.
> > > >
> > > > This patch checks for sk_fullsock() before returning. If it is not
> > > > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > > >
> > > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > > > Cc: Joe Stringer <joe@isovalent.com>
> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > ---
> > > >  net/core/filter.c | 16 ++++++++++++++--
> > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index 55bfc941d17a..85def5a20aaf 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
> > bpf_sock_tuple *tuple, u32 len,
> > > >     struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> > > >                                        ifindex, proto, netns_id,
> > flags);
> > > >
> > > > -   if (sk)
> > > > +   if (sk) {
> > > >             sk = sk_to_full_sk(sk);
> > > > +           if (!sk_fullsock(sk)) {
> > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > +                           sock_gen_put(sk);
> > >
> > > This looks a bit convoluted/weird.
> > >
> > > What about telling/asking __bpf_skc_lookup() to not return a non
> > fullsock instead ?
> > It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
> > can return non fullsock
> >
> 
> FYI this is necessary for finding a transparently proxied socket for a
> non-local connection (tproxy use case).
You meant it is necessary to return a non fullsock from the
BPF_FUNC_sk_lookup_xxx helpers?

> 
> 
> > >
> > > > +                   return NULL;
> > > > +           }
> > > > +   }
> > > >
> > > >     return sk;
> > > >  }
> > > > @@ -5369,8 +5375,14 @@ bpf_sk_lookup(struct sk_buff *skb, struct
> > bpf_sock_tuple *tuple, u32 len,
> > > >     struct sock *sk = bpf_skc_lookup(skb, tuple, len, proto, netns_id,
> > > >                                      flags);
> > > >
> > > > -   if (sk)
> > > > +   if (sk) {
> > > >             sk = sk_to_full_sk(sk);
> > > > +           if (!sk_fullsock(sk)) {
> > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > +                           sock_gen_put(sk);
> > > > +                   return NULL;
> > > > +           }
> > > > +   }
> > > >
> > > >     return sk;
> > > >  }
> > > >
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-18 19:05       ` Martin Lau
@ 2019-05-19  1:52         ` Joe Stringer
  2019-05-19  2:07           ` Martin Lau
  0 siblings, 1 reply; 10+ messages in thread
From: Joe Stringer @ 2019-05-19  1:52 UTC (permalink / raw)
  To: Martin Lau
  Cc: Eric Dumazet, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
	Kernel Team

On Sat, May 18, 2019, 09:05 Martin Lau <kafai@fb.com> wrote:
>
> On Sat, May 18, 2019 at 08:38:46AM -1000, Joe Stringer wrote:
> > On Fri, May 17, 2019, 12:02 Martin Lau <kafai@fb.com> wrote:
> >
> > > On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> > > >
> > > >
> > > > On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > > > > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > > > > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > > > > accessed, e.g. type, protocol, mark and priority.
> > > > > Some new helper, like bpf_sk_storage_get(), also expects
> > > > > ARG_PTR_TO_SOCKET is a fullsock.
> > > > >
> > > > > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > > > > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > > > > to be a fullsock.  For example, it cannot get a fullsock if sk
> > > > > is in TCP_TIME_WAIT.
> > > > >
> > > > > This patch checks for sk_fullsock() before returning. If it is not
> > > > > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > > > >
> > > > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > > > > Cc: Joe Stringer <joe@isovalent.com>
> > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > > ---
> > > > >  net/core/filter.c | 16 ++++++++++++++--
> > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > index 55bfc941d17a..85def5a20aaf 100644
> > > > > --- a/net/core/filter.c
> > > > > +++ b/net/core/filter.c
> > > > > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
> > > bpf_sock_tuple *tuple, u32 len,
> > > > >     struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> > > > >                                        ifindex, proto, netns_id,
> > > flags);
> > > > >
> > > > > -   if (sk)
> > > > > +   if (sk) {
> > > > >             sk = sk_to_full_sk(sk);
> > > > > +           if (!sk_fullsock(sk)) {
> > > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > > +                           sock_gen_put(sk);
> > > >
> > > > This looks a bit convoluted/weird.
> > > >
> > > > What about telling/asking __bpf_skc_lookup() to not return a non
> > > fullsock instead ?
> > > It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
> > > can return non fullsock
> > >
> >
> > FYI this is necessary for finding a transparently proxied socket for a
> > non-local connection (tproxy use case).
> You meant it is necessary to return a non fullsock from the
> BPF_FUNC_sk_lookup_xxx helpers?

Yes, that's what I want to associate with the skb so that the delivery
to the SO_TRANSPARENT is received properly.

For the first packet of a connection, we look up the socket using the
tproxy socket port as the destination, and deliver the packet there.
The SO_TRANSPARENT logic then kicks in and sends back the ack and
creates the non-full sock for the connection tuple, which can be
entirely unrelated to local addresses or ports.

For the second forward-direction packet, (ie ACK in 3-way handshake)
then we must deliver the packet to this non-full sock as that's what
is negotiating the proxied connection. If you look up using the packet
tuple then get the full sock from it, it will go back to the
SO_TRANSPARENT parent socket. Delivering the ACK there will result in
a RST being sent back, because the SO_TRANSPARENT socket is just there
to accept new connections for connections to be proxied. So this is
the case where I need the non-full sock.

(In practice, the lookup logic attempts the packet tuple first then if
that fails, uses the tproxy port for lookup to achieve the above).

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-19  1:52         ` Joe Stringer
@ 2019-05-19  2:07           ` Martin Lau
  2019-05-20 18:38             ` Martin Lau
  2019-05-20 18:56             ` Joe Stringer
  0 siblings, 2 replies; 10+ messages in thread
From: Martin Lau @ 2019-05-19  2:07 UTC (permalink / raw)
  To: Joe Stringer
  Cc: Eric Dumazet, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
	Kernel Team

On Sat, May 18, 2019 at 06:52:48PM -0700, Joe Stringer wrote:
> On Sat, May 18, 2019, 09:05 Martin Lau <kafai@fb.com> wrote:
> >
> > On Sat, May 18, 2019 at 08:38:46AM -1000, Joe Stringer wrote:
> > > On Fri, May 17, 2019, 12:02 Martin Lau <kafai@fb.com> wrote:
> > >
> > > > On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> > > > >
> > > > >
> > > > > On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > > > > > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > > > > > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > > > > > accessed, e.g. type, protocol, mark and priority.
> > > > > > Some new helper, like bpf_sk_storage_get(), also expects
> > > > > > ARG_PTR_TO_SOCKET is a fullsock.
> > > > > >
> > > > > > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > > > > > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > > > > > to be a fullsock.  For example, it cannot get a fullsock if sk
> > > > > > is in TCP_TIME_WAIT.
> > > > > >
> > > > > > This patch checks for sk_fullsock() before returning. If it is not
> > > > > > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > > > > >
> > > > > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > > > > > Cc: Joe Stringer <joe@isovalent.com>
> > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > > > ---
> > > > > >  net/core/filter.c | 16 ++++++++++++++--
> > > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > > index 55bfc941d17a..85def5a20aaf 100644
> > > > > > --- a/net/core/filter.c
> > > > > > +++ b/net/core/filter.c
> > > > > > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
> > > > bpf_sock_tuple *tuple, u32 len,
> > > > > >     struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> > > > > >                                        ifindex, proto, netns_id,
> > > > flags);
> > > > > >
> > > > > > -   if (sk)
> > > > > > +   if (sk) {
> > > > > >             sk = sk_to_full_sk(sk);
> > > > > > +           if (!sk_fullsock(sk)) {
> > > > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > > > +                           sock_gen_put(sk);
> > > > >
> > > > > This looks a bit convoluted/weird.
> > > > >
> > > > > What about telling/asking __bpf_skc_lookup() to not return a non
> > > > fullsock instead ?
> > > > It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
> > > > can return non fullsock
> > > >
> > >
> > > FYI this is necessary for finding a transparently proxied socket for a
> > > non-local connection (tproxy use case).
> > You meant it is necessary to return a non fullsock from the
> > BPF_FUNC_sk_lookup_xxx helpers?
> 
> Yes, that's what I want to associate with the skb so that the delivery
> to the SO_TRANSPARENT is received properly.
> 
> For the first packet of a connection, we look up the socket using the
> tproxy socket port as the destination, and deliver the packet there.
> The SO_TRANSPARENT logic then kicks in and sends back the ack and
> creates the non-full sock for the connection tuple, which can be
> entirely unrelated to local addresses or ports.
> 
> For the second forward-direction packet, (ie ACK in 3-way handshake)
> then we must deliver the packet to this non-full sock as that's what
> is negotiating the proxied connection. If you look up using the packet
> tuple then get the full sock from it, it will go back to the
> SO_TRANSPARENT parent socket. Delivering the ACK there will result in
> a RST being sent back, because the SO_TRANSPARENT socket is just there
> to accept new connections for connections to be proxied. So this is
> the case where I need the non-full sock.
> 
> (In practice, the lookup logic attempts the packet tuple first then if
> that fails, uses the tproxy port for lookup to achieve the above).
hmm...I am likely missing something.

1) The above can be done by the "BPF_FUNC_skC_lookup_tcp" which
   returns a non fullsock (RET_PTR_TO_SOCK_COMMON_OR_NULL), no?

2) The bpf_func_proto of "BPF_FUNC_sk_lookup_tcp" returns
   fullsock (RET_PTR_TO_SOCKET_OR_NULL) and the bpf_prog (and
   the verifier) is expecting that.  How to address the bug here?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-19  2:07           ` Martin Lau
@ 2019-05-20 18:38             ` Martin Lau
  2019-05-20 18:56             ` Joe Stringer
  1 sibling, 0 replies; 10+ messages in thread
From: Martin Lau @ 2019-05-20 18:38 UTC (permalink / raw)
  To: Joe Stringer
  Cc: Eric Dumazet, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
	Kernel Team

On Sat, May 18, 2019 at 07:07:29PM -0700, Martin Lau wrote:
> On Sat, May 18, 2019 at 06:52:48PM -0700, Joe Stringer wrote:
> > On Sat, May 18, 2019, 09:05 Martin Lau <kafai@fb.com> wrote:
> > >
> > > On Sat, May 18, 2019 at 08:38:46AM -1000, Joe Stringer wrote:
> > > > On Fri, May 17, 2019, 12:02 Martin Lau <kafai@fb.com> wrote:
> > > >
> > > > > On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> > > > > >
> > > > > >
> > > > > > On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > > > > > > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > > > > > > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > > > > > > accessed, e.g. type, protocol, mark and priority.
> > > > > > > Some new helper, like bpf_sk_storage_get(), also expects
> > > > > > > ARG_PTR_TO_SOCKET is a fullsock.
> > > > > > >
> > > > > > > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > > > > > > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > > > > > > to be a fullsock.  For example, it cannot get a fullsock if sk
> > > > > > > is in TCP_TIME_WAIT.
> > > > > > >
> > > > > > > This patch checks for sk_fullsock() before returning. If it is not
> > > > > > > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > > > > > >
> > > > > > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > > > > > > Cc: Joe Stringer <joe@isovalent.com>
> > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > > > > ---
> > > > > > >  net/core/filter.c | 16 ++++++++++++++--
> > > > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > > > index 55bfc941d17a..85def5a20aaf 100644
> > > > > > > --- a/net/core/filter.c
> > > > > > > +++ b/net/core/filter.c
> > > > > > > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
> > > > > bpf_sock_tuple *tuple, u32 len,
> > > > > > >     struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> > > > > > >                                        ifindex, proto, netns_id,
> > > > > flags);
> > > > > > >
> > > > > > > -   if (sk)
> > > > > > > +   if (sk) {
> > > > > > >             sk = sk_to_full_sk(sk);
> > > > > > > +           if (!sk_fullsock(sk)) {
> > > > > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > > > > +                           sock_gen_put(sk);
> > > > > >
> > > > > > This looks a bit convoluted/weird.
> > > > > >
> > > > > > What about telling/asking __bpf_skc_lookup() to not return a non
> > > > > fullsock instead ?
> > > > > It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
> > > > > can return non fullsock
> > > > >
> > > >
> > > > FYI this is necessary for finding a transparently proxied socket for a
> > > > non-local connection (tproxy use case).
> > > You meant it is necessary to return a non fullsock from the
> > > BPF_FUNC_sk_lookup_xxx helpers?
> > 
> > Yes, that's what I want to associate with the skb so that the delivery
> > to the SO_TRANSPARENT is received properly.
> > 
> > For the first packet of a connection, we look up the socket using the
> > tproxy socket port as the destination, and deliver the packet there.
> > The SO_TRANSPARENT logic then kicks in and sends back the ack and
> > creates the non-full sock for the connection tuple, which can be
> > entirely unrelated to local addresses or ports.
> > 
> > For the second forward-direction packet, (ie ACK in 3-way handshake)
> > then we must deliver the packet to this non-full sock as that's what
> > is negotiating the proxied connection. If you look up using the packet
> > tuple then get the full sock from it, it will go back to the
> > SO_TRANSPARENT parent socket. Delivering the ACK there will result in
> > a RST being sent back, because the SO_TRANSPARENT socket is just there
> > to accept new connections for connections to be proxied. So this is
> > the case where I need the non-full sock.
> > 
> > (In practice, the lookup logic attempts the packet tuple first then if
> > that fails, uses the tproxy port for lookup to achieve the above).
> hmm...I am likely missing something.
> 
> 1) The above can be done by the "BPF_FUNC_skC_lookup_tcp" which
>    returns a non fullsock (RET_PTR_TO_SOCK_COMMON_OR_NULL), no?
> 
> 2) The bpf_func_proto of "BPF_FUNC_sk_lookup_tcp" returns
>    fullsock (RET_PTR_TO_SOCKET_OR_NULL) and the bpf_prog (and
>    the verifier) is expecting that.  How to address the bug here?
Joe, do you have other concerns on this bug fix?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-19  2:07           ` Martin Lau
  2019-05-20 18:38             ` Martin Lau
@ 2019-05-20 18:56             ` Joe Stringer
  1 sibling, 0 replies; 10+ messages in thread
From: Joe Stringer @ 2019-05-20 18:56 UTC (permalink / raw)
  To: Martin Lau
  Cc: Eric Dumazet, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
	Kernel Team

On Sat, May 18, 2019 at 7:08 PM Martin Lau <kafai@fb.com> wrote:
>
> On Sat, May 18, 2019 at 06:52:48PM -0700, Joe Stringer wrote:
> > On Sat, May 18, 2019, 09:05 Martin Lau <kafai@fb.com> wrote:
> > >
> > > On Sat, May 18, 2019 at 08:38:46AM -1000, Joe Stringer wrote:
> > > > On Fri, May 17, 2019, 12:02 Martin Lau <kafai@fb.com> wrote:
> > > >
> > > > > On Fri, May 17, 2019 at 02:51:48PM -0700, Eric Dumazet wrote:
> > > > > >
> > > > > >
> > > > > > On 5/17/19 2:21 PM, Martin KaFai Lau wrote:
> > > > > > > The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> > > > > > > Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> > > > > > > accessed, e.g. type, protocol, mark and priority.
> > > > > > > Some new helper, like bpf_sk_storage_get(), also expects
> > > > > > > ARG_PTR_TO_SOCKET is a fullsock.
> > > > > > >
> > > > > > > bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> > > > > > > However, the ptr returned from sk_to_full_sk() is not guaranteed
> > > > > > > to be a fullsock.  For example, it cannot get a fullsock if sk
> > > > > > > is in TCP_TIME_WAIT.
> > > > > > >
> > > > > > > This patch checks for sk_fullsock() before returning. If it is not
> > > > > > > a fullsock, sock_gen_put() is called if needed and then returns NULL.
> > > > > > >
> > > > > > > Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> > > > > > > Cc: Joe Stringer <joe@isovalent.com>
> > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > > > > > > ---
> > > > > > >  net/core/filter.c | 16 ++++++++++++++--
> > > > > > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > > > > index 55bfc941d17a..85def5a20aaf 100644
> > > > > > > --- a/net/core/filter.c
> > > > > > > +++ b/net/core/filter.c
> > > > > > > @@ -5337,8 +5337,14 @@ __bpf_sk_lookup(struct sk_buff *skb, struct
> > > > > bpf_sock_tuple *tuple, u32 len,
> > > > > > >     struct sock *sk = __bpf_skc_lookup(skb, tuple, len, caller_net,
> > > > > > >                                        ifindex, proto, netns_id,
> > > > > flags);
> > > > > > >
> > > > > > > -   if (sk)
> > > > > > > +   if (sk) {
> > > > > > >             sk = sk_to_full_sk(sk);
> > > > > > > +           if (!sk_fullsock(sk)) {
> > > > > > > +                   if (!sock_flag(sk, SOCK_RCU_FREE))
> > > > > > > +                           sock_gen_put(sk);
> > > > > >
> > > > > > This looks a bit convoluted/weird.
> > > > > >
> > > > > > What about telling/asking __bpf_skc_lookup() to not return a non
> > > > > fullsock instead ?
> > > > > It is becausee some other helpers, like BPF_FUNC_skc_lookup_tcp,
> > > > > can return non fullsock
> > > > >
> > > >
> > > > FYI this is necessary for finding a transparently proxied socket for a
> > > > non-local connection (tproxy use case).
> > > You meant it is necessary to return a non fullsock from the
> > > BPF_FUNC_sk_lookup_xxx helpers?
> >
> > Yes, that's what I want to associate with the skb so that the delivery
> > to the SO_TRANSPARENT is received properly.
> >
> > For the first packet of a connection, we look up the socket using the
> > tproxy socket port as the destination, and deliver the packet there.
> > The SO_TRANSPARENT logic then kicks in and sends back the ack and
> > creates the non-full sock for the connection tuple, which can be
> > entirely unrelated to local addresses or ports.
> >
> > For the second forward-direction packet, (ie ACK in 3-way handshake)
> > then we must deliver the packet to this non-full sock as that's what
> > is negotiating the proxied connection. If you look up using the packet
> > tuple then get the full sock from it, it will go back to the
> > SO_TRANSPARENT parent socket. Delivering the ACK there will result in
> > a RST being sent back, because the SO_TRANSPARENT socket is just there
> > to accept new connections for connections to be proxied. So this is
> > the case where I need the non-full sock.
> >
> > (In practice, the lookup logic attempts the packet tuple first then if
> > that fails, uses the tproxy port for lookup to achieve the above).
> hmm...I am likely missing something.
>
> 1) The above can be done by the "BPF_FUNC_skC_lookup_tcp" which
>    returns a non fullsock (RET_PTR_TO_SOCK_COMMON_OR_NULL), no?

Correct, I meant to send as response to Eric as to use cases for
__bpf_skc_lookup() returning non fullsock.

> 2) The bpf_func_proto of "BPF_FUNC_sk_lookup_tcp" returns
>    fullsock (RET_PTR_TO_SOCKET_OR_NULL) and the bpf_prog (and
>    the verifier) is expecting that.  How to address the bug here?

Your proposal seems fine to me.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-17 21:21 [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup() Martin KaFai Lau
  2019-05-17 21:51 ` Eric Dumazet
@ 2019-05-20 18:57 ` Joe Stringer
  2019-05-21 14:48 ` Daniel Borkmann
  2 siblings, 0 replies; 10+ messages in thread
From: Joe Stringer @ 2019-05-20 18:57 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Fri, May 17, 2019 at 2:21 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> accessed, e.g. type, protocol, mark and priority.
> Some new helper, like bpf_sk_storage_get(), also expects
> ARG_PTR_TO_SOCKET is a fullsock.
>
> bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> However, the ptr returned from sk_to_full_sk() is not guaranteed
> to be a fullsock.  For example, it cannot get a fullsock if sk
> is in TCP_TIME_WAIT.
>
> This patch checks for sk_fullsock() before returning. If it is not
> a fullsock, sock_gen_put() is called if needed and then returns NULL.
>
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Cc: Joe Stringer <joe@isovalent.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---

Acked-by: Joe Stringer <joe@isovalent.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup()
  2019-05-17 21:21 [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup() Martin KaFai Lau
  2019-05-17 21:51 ` Eric Dumazet
  2019-05-20 18:57 ` Joe Stringer
@ 2019-05-21 14:48 ` Daniel Borkmann
  2 siblings, 0 replies; 10+ messages in thread
From: Daniel Borkmann @ 2019-05-21 14:48 UTC (permalink / raw)
  To: Martin KaFai Lau, bpf, netdev
  Cc: Alexei Starovoitov, kernel-team, Joe Stringer

On 05/17/2019 11:21 PM, Martin KaFai Lau wrote:
> The BPF_FUNC_sk_lookup_xxx helpers return RET_PTR_TO_SOCKET_OR_NULL.
> Meaning a fullsock ptr and its fullsock's fields in bpf_sock can be
> accessed, e.g. type, protocol, mark and priority.
> Some new helper, like bpf_sk_storage_get(), also expects
> ARG_PTR_TO_SOCKET is a fullsock.
> 
> bpf_sk_lookup() currently calls sk_to_full_sk() before returning.
> However, the ptr returned from sk_to_full_sk() is not guaranteed
> to be a fullsock.  For example, it cannot get a fullsock if sk
> is in TCP_TIME_WAIT.
> 
> This patch checks for sk_fullsock() before returning. If it is not
> a fullsock, sock_gen_put() is called if needed and then returns NULL.
> 
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Cc: Joe Stringer <joe@isovalent.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Applied, thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-17 21:21 [PATCH bpf] bpf: Check sk_fullsock() before returning from bpf_sk_lookup() Martin KaFai Lau
2019-05-17 21:51 ` Eric Dumazet
2019-05-17 22:01   ` Martin Lau
     [not found]     ` <CADa=RyxisbcVeXL7yq6o02XOgWd87QCzq-6zDXRnm9RoD2WM=A@mail.gmail.com>
2019-05-18 19:05       ` Martin Lau
2019-05-19  1:52         ` Joe Stringer
2019-05-19  2:07           ` Martin Lau
2019-05-20 18:38             ` Martin Lau
2019-05-20 18:56             ` Joe Stringer
2019-05-20 18:57 ` Joe Stringer
2019-05-21 14:48 ` Daniel Borkmann

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox