[PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Yafang Shao]

Then we can enable/disable socket debugging without modifying user code.
That is more convenient for debugging.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/net/sock.h | 8 ++++++++
 net/core/filter.c  | 3 +++
 net/core/sock.c    | 8 --------
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 2b229f7..8decee9 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
 	}
 }
 
+static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
+{
+	if (valbool)
+		sock_set_flag(sk, bit);
+	else
+		sock_reset_flag(sk, bit);
+}
+
 bool sk_mc_loop(struct sock *sk);
 
 static inline bool sk_can_gso(const struct sock *sk)
diff --git a/net/core/filter.c b/net/core/filter.c
index 3a49f68..ce5da57 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
 
 		/* Only some socketops are supported */
 		switch (optname) {
+		case SO_DEBUG:
+			sock_valbool_flag(sk, SOCK_DBG, val);
+			break;
 		case SO_RCVBUF:
 			sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
 			sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
diff --git a/net/core/sock.c b/net/core/sock.c
index 900e8a9..5ef6daa 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -638,14 +638,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
 	return ret;
 }
 
-static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
-{
-	if (valbool)
-		sock_set_flag(sk, bit);
-	else
-		sock_reset_flag(sk, bit);
-}
-
 bool sk_mc_loop(struct sock *sk)
 {
 	if (dev_recursion_level())
-- 
1.8.3.1

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Y Song]

On Sun, Feb 3, 2019 at 12:18 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>  include/net/sock.h | 8 ++++++++
>  net/core/filter.c  | 3 +++
>  net/core/sock.c    | 8 --------
>  3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 2b229f7..8decee9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
>         }
>  }
>
> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> +{
> +       if (valbool)
> +               sock_set_flag(sk, bit);
> +       else
> +               sock_reset_flag(sk, bit);
> +}
> +
>  bool sk_mc_loop(struct sock *sk);
>
>  static inline bool sk_can_gso(const struct sock *sk)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3a49f68..ce5da57 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>
>                 /* Only some socketops are supported */
>                 switch (optname) {
> +               case SO_DEBUG:
> +                       sock_valbool_flag(sk, SOCK_DBG, val);
> +                       break;
>                 case SO_RCVBUF:
>                         sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
>                         sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 900e8a9..5ef6daa 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -638,14 +638,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
>         return ret;
>  }
>
> -static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> -{
> -       if (valbool)
> -               sock_set_flag(sk, bit);
> -       else
> -               sock_reset_flag(sk, bit);
> -}
> -
>  bool sk_mc_loop(struct sock *sk)
>  {
>         if (dev_recursion_level())
> --
> 1.8.3.1
>

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Lawrence Brakmo]

On 2/3/19, 12:15 AM, "Yafang Shao" <laoar.shao@gmail.com> wrote:

    Then we can enable/disable socket debugging without modifying user code.
    That is more convenient for debugging.
    
    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Acked-by: Lawrence Brakmo <brakmo@fb.com>

    ---
     include/net/sock.h | 8 ++++++++
     net/core/filter.c  | 3 +++
     net/core/sock.c    | 8 --------
     3 files changed, 11 insertions(+), 8 deletions(-)
    
    diff --git a/include/net/sock.h b/include/net/sock.h
    index 2b229f7..8decee9 100644
    --- a/include/net/sock.h
    +++ b/include/net/sock.h
    @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
     	}
     }
     
    +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
    +{
    +	if (valbool)
    +		sock_set_flag(sk, bit);
    +	else
    +		sock_reset_flag(sk, bit);
    +}
    +
     bool sk_mc_loop(struct sock *sk);
     
     static inline bool sk_can_gso(const struct sock *sk)
    diff --git a/net/core/filter.c b/net/core/filter.c
    index 3a49f68..ce5da57 100644
    --- a/net/core/filter.c
    +++ b/net/core/filter.c
    @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
     
     		/* Only some socketops are supported */
     		switch (optname) {
    +		case SO_DEBUG:
    +			sock_valbool_flag(sk, SOCK_DBG, val);
    +			break;
     		case SO_RCVBUF:
     			sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
     			sk->sk_rcvbuf = max_t(int, val * 2, SOCK_MIN_RCVBUF);
    diff --git a/net/core/sock.c b/net/core/sock.c
    index 900e8a9..5ef6daa 100644
    --- a/net/core/sock.c
    +++ b/net/core/sock.c
    @@ -638,14 +638,6 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
     	return ret;
     }
     
    -static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
    -{
    -	if (valbool)
    -		sock_set_flag(sk, bit);
    -	else
    -		sock_reset_flag(sk, bit);
    -}
    -
     bool sk_mc_loop(struct sock *sk)
     {
     	if (dev_recursion_level())
    -- 
    1.8.3.1
    
    

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Quentin Monnet]

2019-02-03 16:15 UTC+0800 ~ Yafang Shao <laoar.shao@gmail.com>
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

Hi Yafang,

The list of socketopts supported by bpf_setsockopt() is described in the
documentation for this BPF helper in include/uapi/linux/bpf.h. Could you
please update that documentation, too?

Thanks,
Quentin

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Alexei Starovoitov]

On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> Then we can enable/disable socket debugging without modifying user code.
> That is more convenient for debugging.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  include/net/sock.h | 8 ++++++++
>  net/core/filter.c  | 3 +++
>  net/core/sock.c    | 8 --------
>  3 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 2b229f7..8decee9 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
>  	}
>  }
>  
> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> +{
> +	if (valbool)
> +		sock_set_flag(sk, bit);
> +	else
> +		sock_reset_flag(sk, bit);
> +}
> +
>  bool sk_mc_loop(struct sock *sk);
>  
>  static inline bool sk_can_gso(const struct sock *sk)
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3a49f68..ce5da57 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>  
>  		/* Only some socketops are supported */
>  		switch (optname) {
> +		case SO_DEBUG:
> +			sock_valbool_flag(sk, SOCK_DBG, val);
> +			break;

I'm missing the point here.
This flag has any effect only when SOCK_DEBUGGING is set.
But it is off in distros.
Since it's for custom debug kernel only why bother with
setting the flag via bpf prog?

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Daniel Borkmann]

On 02/04/2019 06:35 PM, Alexei Starovoitov wrote:
> On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
>> Then we can enable/disable socket debugging without modifying user code.
>> That is more convenient for debugging.
>>
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> ---
>>  include/net/sock.h | 8 ++++++++
>>  net/core/filter.c  | 3 +++
>>  net/core/sock.c    | 8 --------
>>  3 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/sock.h b/include/net/sock.h
>> index 2b229f7..8decee9 100644
>> --- a/include/net/sock.h
>> +++ b/include/net/sock.h
>> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
>>  	}
>>  }
>>  
>> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
>> +{
>> +	if (valbool)
>> +		sock_set_flag(sk, bit);
>> +	else
>> +		sock_reset_flag(sk, bit);
>> +}
>> +
>>  bool sk_mc_loop(struct sock *sk);
>>  
>>  static inline bool sk_can_gso(const struct sock *sk)
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 3a49f68..ce5da57 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
>>  
>>  		/* Only some socketops are supported */
>>  		switch (optname) {
>> +		case SO_DEBUG:
>> +			sock_valbool_flag(sk, SOCK_DBG, val);
>> +			break;
> 
> I'm missing the point here.
> This flag has any effect only when SOCK_DEBUGGING is set.
> But it is off in distros.
> Since it's for custom debug kernel only why bother with
> setting the flag via bpf prog?

+1, this seems like some ancient debugging interface. Back at last netconf
there was a proposal [0] to have a tcp_stats(sk, TCP_MIB_...) API for MIBs
counter such that this can be traced via BPF on a per socket basis, for
example. Might be worthwhile to work into that direction instead and potentially
get rid of the SOCK_DEBUG() statements and convert (where appropriate) to
such an interface. Thoughts?

  [0] page 14, http://vger.kernel.org/netconf2018_files/BrendanGregg_netconf2018.pdf

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Yafang Shao]

On Tue, Feb 5, 2019 at 1:35 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> > Then we can enable/disable socket debugging without modifying user code.
> > That is more convenient for debugging.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/net/sock.h | 8 ++++++++
> >  net/core/filter.c  | 3 +++
> >  net/core/sock.c    | 8 --------
> >  3 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 2b229f7..8decee9 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> >       }
> >  }
> >
> > +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> > +{
> > +     if (valbool)
> > +             sock_set_flag(sk, bit);
> > +     else
> > +             sock_reset_flag(sk, bit);
> > +}
> > +
> >  bool sk_mc_loop(struct sock *sk);
> >
> >  static inline bool sk_can_gso(const struct sock *sk)
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 3a49f68..ce5da57 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> >
> >               /* Only some socketops are supported */
> >               switch (optname) {
> > +             case SO_DEBUG:
> > +                     sock_valbool_flag(sk, SOCK_DBG, val);
> > +                     break;
>
> I'm missing the point here.
> This flag has any effect only when SOCK_DEBUGGING is set.
> But it is off in distros.

It's not off in distros. At least it is set in RHEL.
Pls. see the comment in include/net/sock.h,
    /* Define this to get the SOCK_DBG debugging facility. */
    #define SOCK_DEBUGGING

> Since it's for custom debug kernel only why bother with
> setting the flag via bpf prog?
>

Re: [PATCH bpf-next] bpf: support SO_DEBUG in bpf_setsockopt()

[Yafang Shao]

On Tue, Feb 5, 2019 at 4:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/04/2019 06:35 PM, Alexei Starovoitov wrote:
> > On Sun, Feb 03, 2019 at 04:15:07PM +0800, Yafang Shao wrote:
> >> Then we can enable/disable socket debugging without modifying user code.
> >> That is more convenient for debugging.
> >>
> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >> ---
> >>  include/net/sock.h | 8 ++++++++
> >>  net/core/filter.c  | 3 +++
> >>  net/core/sock.c    | 8 --------
> >>  3 files changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 2b229f7..8decee9 100644
> >> --- a/include/net/sock.h
> >> +++ b/include/net/sock.h
> >> @@ -1935,6 +1935,14 @@ static inline void sock_confirm_neigh(struct sk_buff *skb, struct neighbour *n)
> >>      }
> >>  }
> >>
> >> +static inline void sock_valbool_flag(struct sock *sk, int bit, int valbool)
> >> +{
> >> +    if (valbool)
> >> +            sock_set_flag(sk, bit);
> >> +    else
> >> +            sock_reset_flag(sk, bit);
> >> +}
> >> +
> >>  bool sk_mc_loop(struct sock *sk);
> >>
> >>  static inline bool sk_can_gso(const struct sock *sk)
> >> diff --git a/net/core/filter.c b/net/core/filter.c
> >> index 3a49f68..ce5da57 100644
> >> --- a/net/core/filter.c
> >> +++ b/net/core/filter.c
> >> @@ -4111,6 +4111,9 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> >>
> >>              /* Only some socketops are supported */
> >>              switch (optname) {
> >> +            case SO_DEBUG:
> >> +                    sock_valbool_flag(sk, SOCK_DBG, val);
> >> +                    break;
> >
> > I'm missing the point here.
> > This flag has any effect only when SOCK_DEBUGGING is set.
> > But it is off in distros.
> > Since it's for custom debug kernel only why bother with
> > setting the flag via bpf prog?
>
> +1, this seems like some ancient debugging interface. Back at last netconf
> there was a proposal [0] to have a tcp_stats(sk, TCP_MIB_...) API for MIBs
> counter such that this can be traced via BPF on a per socket basis, for
> example. Might be worthwhile to work into that direction instead and potentially
> get rid of the SOCK_DEBUG() statements and convert (where appropriate) to
> such an interface. Thoughts?
>
>   [0] page 14, http://vger.kernel.org/netconf2018_files/BrendanGregg_netconf2018.pdf

This proposal seems like a better solution.
I will think about it.
Thanks for your suggestion.

Thanks
Yafang