Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[John Fastabend]

Forrest Chen wrote:
> Thanks John for the review.

Usually try not to top post.

> 
> I think you are right for the missing part. I would like to update the
> patch and re-send it, is it ok for you?

Yep glad to have more folks looking over sockmap code and looks like it
already hit the list so that is good.

> 
> Since it is my first kernel patch, I'm not familiar with the process.
> What''s the meaning of 'add your Signed-off-by'?
> I think I have add  Signed-off-by in the first patch, do you mean I should
> add 'Signed-off-by: John Fastabend <john.fastabend@gmail.com>' as well?
> 

I was just saying feel free to add _your_ Signed-off-by to the patch I
attached and send it. You added my signed-off-by to your patch which is
also fine I don't think it matters much as long as we get the fix.

Also we should add Fixes tag and a tag to give Arika some credit. Seems
we had the same fix in mind. I'll just add those now, next time for
fixes please add a Fixes tag so we can track where we need to backport
the fix.

Thanks for looking into this.

[PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Lingpeng Chen]

Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
and there's also some data in psock->ingress_msg, the data in
psock->ingress_msg will be purged. It is always happen when request to a
HTTP1.0 server like python SimpleHTTPServer since the server send FIN
packet after data is sent out.

Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
Reported-by: Arika Chen <eaglesora@gmail.com>
Suggested-by: Arika Chen <eaglesora@gmail.com>
Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/ipv4/tcp_bpf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index e38705165ac9..f7e902868fce 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -123,12 +123,13 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 
 	if (unlikely(flags & MSG_ERRQUEUE))
 		return inet_recv_error(sk, msg, len, addr_len);
-	if (!skb_queue_empty(&sk->sk_receive_queue))
-		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
 
 	psock = sk_psock_get(sk);
 	if (unlikely(!psock))
 		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
+	if (!skb_queue_empty(&sk->sk_receive_queue) &&
+	    sk_psock_queue_empty(psock))
+		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
 	lock_sock(sk);
 msg_bytes_ready:
 	copied = __tcp_bpf_recvmsg(sk, psock, msg, len, flags);
@@ -139,7 +140,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		timeo = sock_rcvtimeo(sk, nonblock);
 		data = tcp_bpf_wait_data(sk, psock, flags, timeo, &err);
 		if (data) {
-			if (skb_queue_empty(&sk->sk_receive_queue))
+			if (!sk_psock_queue_empty(psock))
 				goto msg_bytes_ready;
 			release_sock(sk);
 			sk_psock_put(sk, psock);
-- 
2.17.1

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Song Liu]

On Tue, Jan 7, 2020 at 8:58 PM Lingpeng Chen <forrest0579@gmail.com> wrote:
>
> Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
> if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
> and there's also some data in psock->ingress_msg, the data in
> psock->ingress_msg will be purged. It is always happen when request to a
> HTTP1.0 server like python SimpleHTTPServer since the server send FIN
> packet after data is sent out.
>
> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> Reported-by: Arika Chen <eaglesora@gmail.com>
> Suggested-by: Arika Chen <eaglesora@gmail.com>
> Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

Cc: stable@vger.kernel.org # v4.20+
Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Daniel Borkmann]

On Wed, Jan 08, 2020 at 12:57:08PM +0800, Lingpeng Chen wrote:
> Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
> if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
> and there's also some data in psock->ingress_msg, the data in
> psock->ingress_msg will be purged. It is always happen when request to a
> HTTP1.0 server like python SimpleHTTPServer since the server send FIN
> packet after data is sent out.
> 
> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> Reported-by: Arika Chen <eaglesora@gmail.com>
> Suggested-by: Arika Chen <eaglesora@gmail.com>
> Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
>  net/ipv4/tcp_bpf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index e38705165ac9..f7e902868fce 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -123,12 +123,13 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  
>  	if (unlikely(flags & MSG_ERRQUEUE))
>  		return inet_recv_error(sk, msg, len, addr_len);

Shouldn't we also move the error queue handling below the psock test as
well and let tcp_recvmsg() natively do it in case of !psock?

> -	if (!skb_queue_empty(&sk->sk_receive_queue))
> -		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>  
>  	psock = sk_psock_get(sk);
>  	if (unlikely(!psock))
>  		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
> +	if (!skb_queue_empty(&sk->sk_receive_queue) &&
> +	    sk_psock_queue_empty(psock))
> +		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>  	lock_sock(sk);
>  msg_bytes_ready:
>  	copied = __tcp_bpf_recvmsg(sk, psock, msg, len, flags);
> @@ -139,7 +140,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  		timeo = sock_rcvtimeo(sk, nonblock);
>  		data = tcp_bpf_wait_data(sk, psock, flags, timeo, &err);
>  		if (data) {
> -			if (skb_queue_empty(&sk->sk_receive_queue))
> +			if (!sk_psock_queue_empty(psock))
>  				goto msg_bytes_ready;
>  			release_sock(sk);
>  			sk_psock_put(sk, psock);
> -- 
> 2.17.1
> 

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[John Fastabend]

Daniel Borkmann wrote:
> On Wed, Jan 08, 2020 at 12:57:08PM +0800, Lingpeng Chen wrote:
> > Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
> > if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
> > and there's also some data in psock->ingress_msg, the data in
> > psock->ingress_msg will be purged. It is always happen when request to a
> > HTTP1.0 server like python SimpleHTTPServer since the server send FIN
> > packet after data is sent out.
> > 
> > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> > Reported-by: Arika Chen <eaglesora@gmail.com>
> > Suggested-by: Arika Chen <eaglesora@gmail.com>
> > Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> >  net/ipv4/tcp_bpf.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> > index e38705165ac9..f7e902868fce 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -123,12 +123,13 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  
> >  	if (unlikely(flags & MSG_ERRQUEUE))
> >  		return inet_recv_error(sk, msg, len, addr_len);
> 
> Shouldn't we also move the error queue handling below the psock test as
> well and let tcp_recvmsg() natively do it in case of !psock?
> 

You mean the MSG_ERRQUEUE flag handling? If the user sets MSG_ERRQUEUE
they expect to receive any queued errors it would be wrong to return
psock data in this case if psock is attached and has data on queue and
user passes MSG_ERRQUEUE flag.

 MSG_ERRQUEUE (since Linux 2.2)
  This flag specifies that queued errors should be received from the socket
  error queue.  The error is passed in an ancillary message with a type
  dependent on the protocol (for IPv4 IP_RECVERR).  The user should supply
  a buffer of sufficient size. See cmsg(3) and ip(7) for more information.
  The payload of the original packet that caused the error is passed as
  normal data via msg_iovec. The original destination address of the
  datagram that caused the error is supplied via msg_name.

I believe it needs to be where it is.


> > -	if (!skb_queue_empty(&sk->sk_receive_queue))
> > -		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
> >  
> >  	psock = sk_psock_get(sk);
> >  	if (unlikely(!psock))
> >  		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
> > +	if (!skb_queue_empty(&sk->sk_receive_queue) &&
> > +	    sk_psock_queue_empty(psock))
> > +		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
> >  	lock_sock(sk);
> >  msg_bytes_ready:
> >  	copied = __tcp_bpf_recvmsg(sk, psock, msg, len, flags);
> > @@ -139,7 +140,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >  		timeo = sock_rcvtimeo(sk, nonblock);
> >  		data = tcp_bpf_wait_data(sk, psock, flags, timeo, &err);
> >  		if (data) {
> > -			if (skb_queue_empty(&sk->sk_receive_queue))
> > +			if (!sk_psock_queue_empty(psock))
> >  				goto msg_bytes_ready;
> >  			release_sock(sk);
> >  			sk_psock_put(sk, psock);
> > -- 
> > 2.17.1
> > 

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Daniel Borkmann]

On 1/8/20 7:01 PM, John Fastabend wrote:
> Daniel Borkmann wrote:
>> On Wed, Jan 08, 2020 at 12:57:08PM +0800, Lingpeng Chen wrote:
>>> Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
>>> if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
>>> and there's also some data in psock->ingress_msg, the data in
>>> psock->ingress_msg will be purged. It is always happen when request to a
>>> HTTP1.0 server like python SimpleHTTPServer since the server send FIN
>>> packet after data is sent out.
>>>
>>> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
>>> Reported-by: Arika Chen <eaglesora@gmail.com>
>>> Suggested-by: Arika Chen <eaglesora@gmail.com>
>>> Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
>>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>>> ---
>>>   net/ipv4/tcp_bpf.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
>>> index e38705165ac9..f7e902868fce 100644
>>> --- a/net/ipv4/tcp_bpf.c
>>> +++ b/net/ipv4/tcp_bpf.c
>>> @@ -123,12 +123,13 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>>>   
>>>   	if (unlikely(flags & MSG_ERRQUEUE))
>>>   		return inet_recv_error(sk, msg, len, addr_len);
>>
>> Shouldn't we also move the error queue handling below the psock test as
>> well and let tcp_recvmsg() natively do it in case of !psock?
>>
> 
> You mean the MSG_ERRQUEUE flag handling? If the user sets MSG_ERRQUEUE
> they expect to receive any queued errors it would be wrong to return
> psock data in this case if psock is attached and has data on queue and
> user passes MSG_ERRQUEUE flag.
> 
>   MSG_ERRQUEUE (since Linux 2.2)
>    This flag specifies that queued errors should be received from the socket
>    error queue.  The error is passed in an ancillary message with a type
>    dependent on the protocol (for IPv4 IP_RECVERR).  The user should supply
>    a buffer of sufficient size. See cmsg(3) and ip(7) for more information.
>    The payload of the original packet that caused the error is passed as
>    normal data via msg_iovec. The original destination address of the
>    datagram that caused the error is supplied via msg_name.
> 
> I believe it needs to be where it is.

I meant that it should have looked as follows (aka moving both below the
psock test) ...

         psock = sk_psock_get(sk);
         if (unlikely(!psock))
             return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
         if (unlikely(flags & MSG_ERRQUEUE))
             return inet_recv_error(sk, msg, len, addr_len);
	if (!skb_queue_empty(&sk->sk_receive_queue) && [...]

... since when detached it's handled already via tcp_recvmsg() internals.

>>> -	if (!skb_queue_empty(&sk->sk_receive_queue))
>>> -		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>>>   
>>>   	psock = sk_psock_get(sk);
>>>   	if (unlikely(!psock))
>>>   		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>>> +	if (!skb_queue_empty(&sk->sk_receive_queue) &&
>>> +	    sk_psock_queue_empty(psock))
>>> +		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>>>   	lock_sock(sk);
>>>   msg_bytes_ready:
>>>   	copied = __tcp_bpf_recvmsg(sk, psock, msg, len, flags);
>>> @@ -139,7 +140,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>>>   		timeo = sock_rcvtimeo(sk, nonblock);
>>>   		data = tcp_bpf_wait_data(sk, psock, flags, timeo, &err);
>>>   		if (data) {
>>> -			if (skb_queue_empty(&sk->sk_receive_queue))
>>> +			if (!sk_psock_queue_empty(psock))
>>>   				goto msg_bytes_ready;
>>>   			release_sock(sk);
>>>   			sk_psock_put(sk, psock);
>>> -- 
>>> 2.17.1
>>>
> 
> 

Re: [PATCH] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[John Fastabend]

Daniel Borkmann wrote:
> On 1/8/20 7:01 PM, John Fastabend wrote:
> > Daniel Borkmann wrote:
> >> On Wed, Jan 08, 2020 at 12:57:08PM +0800, Lingpeng Chen wrote:
> >>> Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
> >>> if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
> >>> and there's also some data in psock->ingress_msg, the data in
> >>> psock->ingress_msg will be purged. It is always happen when request to a
> >>> HTTP1.0 server like python SimpleHTTPServer since the server send FIN
> >>> packet after data is sent out.
> >>>
> >>> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> >>> Reported-by: Arika Chen <eaglesora@gmail.com>
> >>> Suggested-by: Arika Chen <eaglesora@gmail.com>
> >>> Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
> >>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> >>> ---
> >>>   net/ipv4/tcp_bpf.c | 7 ++++---
> >>>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> >>> index e38705165ac9..f7e902868fce 100644
> >>> --- a/net/ipv4/tcp_bpf.c
> >>> +++ b/net/ipv4/tcp_bpf.c
> >>> @@ -123,12 +123,13 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >>>   
> >>>   	if (unlikely(flags & MSG_ERRQUEUE))
> >>>   		return inet_recv_error(sk, msg, len, addr_len);
> >>
> >> Shouldn't we also move the error queue handling below the psock test as
> >> well and let tcp_recvmsg() natively do it in case of !psock?
> >>
> > 
> > You mean the MSG_ERRQUEUE flag handling? If the user sets MSG_ERRQUEUE
> > they expect to receive any queued errors it would be wrong to return
> > psock data in this case if psock is attached and has data on queue and
> > user passes MSG_ERRQUEUE flag.
> > 
> >   MSG_ERRQUEUE (since Linux 2.2)
> >    This flag specifies that queued errors should be received from the socket
> >    error queue.  The error is passed in an ancillary message with a type
> >    dependent on the protocol (for IPv4 IP_RECVERR).  The user should supply
> >    a buffer of sufficient size. See cmsg(3) and ip(7) for more information.
> >    The payload of the original packet that caused the error is passed as
> >    normal data via msg_iovec. The original destination address of the
> >    datagram that caused the error is supplied via msg_name.
> > 
> > I believe it needs to be where it is.
> 
> I meant that it should have looked as follows (aka moving both below the
> psock test) ...
> 
>          psock = sk_psock_get(sk);
>          if (unlikely(!psock))
>              return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
>          if (unlikely(flags & MSG_ERRQUEUE))
>              return inet_recv_error(sk, msg, len, addr_len);
> 	if (!skb_queue_empty(&sk->sk_receive_queue) && [...]
> 
> ... since when detached it's handled already via tcp_recvmsg() internals.
> 

Ah, OK the 'if (unlikely(!psock))' is a very rare case so I'm not sure
it matters much. But, I agree it is slightly nicer.

Lingpeng, can you spin a v2 moving the MSG_ERRQUEUE check down and also
keep the ACK and stable tags I don't think its a very big change the
Acks can stick around IMO.

Thanks.

[PATCH v2] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Lingpeng Chen]

Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
and there's also some data in psock->ingress_msg, the data in
psock->ingress_msg will be purged. It is always happen when request to a
HTTP1.0 server like python SimpleHTTPServer since the server send FIN
packet after data is sent out.

Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
Reported-by: Arika Chen <eaglesora@gmail.com>
Suggested-by: Arika Chen <eaglesora@gmail.com>
Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Cc: stable@vger.kernel.org # v4.20+
Acked-by: Song Liu <songliubraving@fb.com>
---
 net/ipv4/tcp_bpf.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index e38705165ac9..e6b08b5a0895 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -121,14 +121,14 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	struct sk_psock *psock;
 	int copied, ret;
 
-	if (unlikely(flags & MSG_ERRQUEUE))
-		return inet_recv_error(sk, msg, len, addr_len);
-	if (!skb_queue_empty(&sk->sk_receive_queue))
-		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
-
 	psock = sk_psock_get(sk);
 	if (unlikely(!psock))
 		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
+	if (unlikely(flags & MSG_ERRQUEUE))
+		return inet_recv_error(sk, msg, len, addr_len);
+	if (!skb_queue_empty(&sk->sk_receive_queue) &&
+	    sk_psock_queue_empty(psock))
+		return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
 	lock_sock(sk);
 msg_bytes_ready:
 	copied = __tcp_bpf_recvmsg(sk, psock, msg, len, flags);
@@ -139,7 +139,7 @@ int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 		timeo = sock_rcvtimeo(sk, nonblock);
 		data = tcp_bpf_wait_data(sk, psock, flags, timeo, &err);
 		if (data) {
-			if (skb_queue_empty(&sk->sk_receive_queue))
+			if (!sk_psock_queue_empty(psock))
 				goto msg_bytes_ready;
 			release_sock(sk);
 			sk_psock_put(sk, psock);
-- 
2.17.1

Re: [PATCH v2] bpf/sockmap: read psock ingress_msg before sk_receive_queue

[Daniel Borkmann]

On 1/9/20 2:48 AM, Lingpeng Chen wrote:
> Right now in tcp_bpf_recvmsg, sock read data first from sk_receive_queue
> if not empty than psock->ingress_msg otherwise. If a FIN packet arrives
> and there's also some data in psock->ingress_msg, the data in
> psock->ingress_msg will be purged. It is always happen when request to a
> HTTP1.0 server like python SimpleHTTPServer since the server send FIN
> packet after data is sent out.
> 
> Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface")
> Reported-by: Arika Chen <eaglesora@gmail.com>
> Suggested-by: Arika Chen <eaglesora@gmail.com>
> Signed-off-by: Lingpeng Chen <forrest0579@gmail.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> Cc: stable@vger.kernel.org # v4.20+
> Acked-by: Song Liu <songliubraving@fb.com>

Applied to bpf, thanks!