Currently, only TCP supports strparser, but sockmap doesn't intercept
non-TCP to attach strparser. For example, with UDP, although the
read/write handlers are replaced, strparser is not executed due to the
lack of read_sock operation.

Furthermore, in udp_bpf_recvmsg(), it checks whether psock has data, and
if not, it falls back to the native UDP read interface, making
UDP + strparser appear to read correctly. According to it's commit
history, the behavior is unexpected.

Moreover, since UDP lacks the concept of streams, we intercept it
directly. Later, we will try to support Unix streams and add more
check.

Signed-off-by: Jiayuan Chen <mrpre@163.com>
---
 net/core/sock_map.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index f1b9b3958792..c6ee2d1d9cf2 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -214,6 +214,14 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
 	return psock;
 }
 
+static bool sock_map_sk_strp_allowed(const struct sock *sk)
+{
+	/* todo: support unix stream socket */
+	if (sk_is_tcp(sk))
+		return true;
+	return false;
+}
+
 static int sock_map_link(struct bpf_map *map, struct sock *sk)
 {
 	struct sk_psock_progs *progs = sock_map_progs(map);
@@ -303,7 +311,10 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 
 	write_lock_bh(&sk->sk_callback_lock);
 	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
-		ret = sk_psock_init_strp(sk, psock);
+		if (sock_map_sk_strp_allowed(sk))
+			ret = sk_psock_init_strp(sk, psock);
+		else
+			ret = -EOPNOTSUPP;
 		if (ret) {
 			write_unlock_bh(&sk->sk_callback_lock);
 			sk_psock_put(sk, psock);
-- 
2.43.5

On Thu, Jan 16, 2025 at 10:05 PM +08, Jiayuan Chen wrote:
> Currently, only TCP supports strparser, but sockmap doesn't intercept
> non-TCP to attach strparser. For example, with UDP, although the
> read/write handlers are replaced, strparser is not executed due to the
> lack of read_sock operation.
>
> Furthermore, in udp_bpf_recvmsg(), it checks whether psock has data, and
> if not, it falls back to the native UDP read interface, making
> UDP + strparser appear to read correctly. According to it's commit
> history, the behavior is unexpected.
>
> Moreover, since UDP lacks the concept of streams, we intercept it
> directly. Later, we will try to support Unix streams and add more
> check.
>
> Signed-off-by: Jiayuan Chen <mrpre@163.com>
> ---

Needs a Fixes: tag.

>  net/core/sock_map.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index f1b9b3958792..c6ee2d1d9cf2 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -214,6 +214,14 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
>  	return psock;
>  }
>  
> +static bool sock_map_sk_strp_allowed(const struct sock *sk)
> +{
> +	/* todo: support unix stream socket */
> +	if (sk_is_tcp(sk))
> +		return true;
> +	return false;
> +}
> +

We don't need this yet, so please don't add it. Especially since this is
fix. It should be kept down to a minimum. Do the sk_is_tcp() check
directly from sock_map_link().

>  static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  {
>  	struct sk_psock_progs *progs = sock_map_progs(map);
> @@ -303,7 +311,10 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  
>  	write_lock_bh(&sk->sk_callback_lock);
>  	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
> -		ret = sk_psock_init_strp(sk, psock);
> +		if (sock_map_sk_strp_allowed(sk))
> +			ret = sk_psock_init_strp(sk, psock);
> +		else
> +			ret = -EOPNOTSUPP;
>  		if (ret) {
>  			write_unlock_bh(&sk->sk_callback_lock);
>  			sk_psock_put(sk, psock);

On Sat, Jan 18, 2025 at 04:03:33PM +0100, Jakub Sitnicki wrote:
> On Thu, Jan 16, 2025 at 10:05 PM +08, Jiayuan Chen wrote:
> > +	if (sk_is_tcp(sk))
> > +		return true;
> > +	return false;
> > +}
> > +
> 
> We don't need this yet, so please don't add it. Especially since this is
> fix. It should be kept down to a minimum. Do the sk_is_tcp() check
> directly from sock_map_link().
> 
Ok, I will do this.
> >  static int sock_map_link(struct bpf_map *map, struct sock *sk)
> >  {
> >  	struct sk_psock_progs *progs = sock_map_progs(map);
> > @@ -303,7 +311,10 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
> >  
> >  	write_lock_bh(&sk->sk_callback_lock);
> >  	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
> > -		ret = sk_psock_init_strp(sk, psock);
> > +		if (sock_map_sk_strp_allowed(sk))
> > +			ret = sk_psock_init_strp(sk, psock);
> > +		else
> > +			ret = -EOPNOTSUPP;
> >  		if (ret) {
> >  			write_unlock_bh(&sk->sk_callback_lock);
> >  			sk_psock_put(sk, psock);