diff mbox series

[net-next,1/3] net-timestamp: add strict check when setting tx flags

Message ID 20240930092416.80830-2-kerneljasonxing@gmail.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series net-timestamp: add some trivial | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 12 this patch: 12
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 9 this patch: 9
netdev/source_inline success Was 0 now: 0

Commit Message

Jason Xing Sept. 30, 2024, 9:24 a.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

Even though this case is unlikely to happen, we have to avoid such
a case occurring at an earlier point: the sk_rmem_alloc could get
increased because of inserting more and more skbs into the errqueue
when calling __skb_complete_tx_timestamp(). This bad case would stop
the socket transmitting soon.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/core/sock.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Willem de Bruijn Sept. 30, 2024, 10:39 a.m. UTC | #1 
Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Even though this case is unlikely to happen, we have to avoid such
> a case occurring at an earlier point: the sk_rmem_alloc could get
> increased because of inserting more and more skbs into the errqueue
> when calling __skb_complete_tx_timestamp(). This bad case would stop
> the socket transmitting soon.

It is up to the application to read from the error queue frequently
enough and/or increase SO_RCVBUF.
 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/core/sock.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index fe87f9bd8f16..4bddd6f62e4f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
>  	if (val & ~SOF_TIMESTAMPING_MASK)
>  		return -EINVAL;
>  
> +	if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> +	    !(val & SOF_TIMESTAMPING_SOFTWARE))
> +		return -EINVAL;
> +

This breaks hardware timestamping

>  	if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
>  	    !(val & SOF_TIMESTAMPING_OPT_ID))
>  		return -EINVAL;
> -- 
> 2.37.3
>
Vadim Fedorenko Sept. 30, 2024, 10:48 a.m. UTC | #2 
On 30/09/2024 10:24, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Even though this case is unlikely to happen, we have to avoid such
> a case occurring at an earlier point: the sk_rmem_alloc could get
> increased because of inserting more and more skbs into the errqueue
> when calling __skb_complete_tx_timestamp(). This bad case would stop
> the socket transmitting soon.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>   net/core/sock.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index fe87f9bd8f16..4bddd6f62e4f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
>   	if (val & ~SOF_TIMESTAMPING_MASK)
>   		return -EINVAL;
>   
> +	if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> +	    !(val & SOF_TIMESTAMPING_SOFTWARE))
> +		return -EINVAL;

SOF_TIMESTAMPING_TX_RECORD_MASK contains SOF_TIMESTAMPING_TX_HARDWARE.
That means that there will be no option to enable HW TX timestamping
without enabling software timestamping. I believe this is wrong
restriction.

> +
>   	if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
>   	    !(val & SOF_TIMESTAMPING_OPT_ID))
>   		return -EINVAL;
Jason Xing Sept. 30, 2024, 11:24 a.m. UTC | #3 
On Mon, Sep 30, 2024 at 6:48 PM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 30/09/2024 10:24, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Even though this case is unlikely to happen, we have to avoid such
> > a case occurring at an earlier point: the sk_rmem_alloc could get
> > increased because of inserting more and more skbs into the errqueue
> > when calling __skb_complete_tx_timestamp(). This bad case would stop
> > the socket transmitting soon.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >   net/core/sock.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index fe87f9bd8f16..4bddd6f62e4f 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >       if (val & ~SOF_TIMESTAMPING_MASK)
> >               return -EINVAL;
> >
> > +     if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> > +         !(val & SOF_TIMESTAMPING_SOFTWARE))
> > +             return -EINVAL;
>
> SOF_TIMESTAMPING_TX_RECORD_MASK contains SOF_TIMESTAMPING_TX_HARDWARE.
> That means that there will be no option to enable HW TX timestamping
> without enabling software timestamping. I believe this is wrong
> restriction.

Thanks! You are right. I should have realized that. I need to get rid
of this one.

Thanks,
Jason
Jason Xing Sept. 30, 2024, 11:29 a.m. UTC | #4 
On Mon, Sep 30, 2024 at 6:39 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Even though this case is unlikely to happen, we have to avoid such
> > a case occurring at an earlier point: the sk_rmem_alloc could get
> > increased because of inserting more and more skbs into the errqueue
> > when calling __skb_complete_tx_timestamp(). This bad case would stop
> > the socket transmitting soon.
>
> It is up to the application to read from the error queue frequently
> enough and/or increase SO_RCVBUF.

Sure thing. If we test it without setting SOF_TIMESTAMPING_SOFTWARE on
the loopback, it will soon stop. That's the reason why I tried to add
the restriction just in case.

>
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  net/core/sock.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index fe87f9bd8f16..4bddd6f62e4f 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
> >       if (val & ~SOF_TIMESTAMPING_MASK)
> >               return -EINVAL;
> >
> > +     if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> > +         !(val & SOF_TIMESTAMPING_SOFTWARE))
> > +             return -EINVAL;
> > +
>
> This breaks hardware timestamping

Yes, and sorry about that. I'll fix this.

>
> >       if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
> >           !(val & SOF_TIMESTAMPING_OPT_ID))
> >               return -EINVAL;
> > --
> > 2.37.3
> >
>
>
Willem de Bruijn Sept. 30, 2024, 11:49 a.m. UTC | #5 
Jason Xing wrote:
> On Mon, Sep 30, 2024 at 6:39 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Even though this case is unlikely to happen, we have to avoid such
> > > a case occurring at an earlier point: the sk_rmem_alloc could get
> > > increased because of inserting more and more skbs into the errqueue
> > > when calling __skb_complete_tx_timestamp(). This bad case would stop
> > > the socket transmitting soon.
> >
> > It is up to the application to read from the error queue frequently
> > enough and/or increase SO_RCVBUF.
> 
> Sure thing. If we test it without setting SOF_TIMESTAMPING_SOFTWARE on
> the loopback, it will soon stop. That's the reason why I tried to add
> the restriction just in case.

I don't follow at all.

That bit does not affect the core issue: that the application is not
clearing its error queue quickly enough.
 
> >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > >  net/core/sock.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index fe87f9bd8f16..4bddd6f62e4f 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> > > @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
> > >       if (val & ~SOF_TIMESTAMPING_MASK)
> > >               return -EINVAL;
> > >
> > > +     if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> > > +         !(val & SOF_TIMESTAMPING_SOFTWARE))
> > > +             return -EINVAL;
> > > +
> >
> > This breaks hardware timestamping
> 
> Yes, and sorry about that. I'll fix this.

As is I don't understand the purpose of this patch. Please do not
just resubmit with a change, but explain the problem and suggested
solution first.

> >
> > >       if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
> > >           !(val & SOF_TIMESTAMPING_OPT_ID))
> > >               return -EINVAL;
> > > --
> > > 2.37.3
> > >
> >
> >
Jason Xing Sept. 30, 2024, 12:42 p.m. UTC | #6 
On Mon, Sep 30, 2024 at 7:49 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Jason Xing wrote:
> > On Mon, Sep 30, 2024 at 6:39 PM Willem de Bruijn
> > <willemdebruijn.kernel@gmail.com> wrote:
> > >
> > > Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Even though this case is unlikely to happen, we have to avoid such
> > > > a case occurring at an earlier point: the sk_rmem_alloc could get
> > > > increased because of inserting more and more skbs into the errqueue
> > > > when calling __skb_complete_tx_timestamp(). This bad case would stop
> > > > the socket transmitting soon.
> > >
> > > It is up to the application to read from the error queue frequently
> > > enough and/or increase SO_RCVBUF.
> >
> > Sure thing. If we test it without setting SOF_TIMESTAMPING_SOFTWARE on
> > the loopback, it will soon stop. That's the reason why I tried to add
> > the restriction just in case.
>
> I don't follow at all.
>
> That bit does not affect the core issue: that the application is not
> clearing its error queue quickly enough.
>
> > >
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > >  net/core/sock.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > > index fe87f9bd8f16..4bddd6f62e4f 100644
> > > > --- a/net/core/sock.c
> > > > +++ b/net/core/sock.c
> > > > @@ -905,6 +905,10 @@ int sock_set_timestamping(struct sock *sk, int optname,
> > > >       if (val & ~SOF_TIMESTAMPING_MASK)
> > > >               return -EINVAL;
> > > >
> > > > +     if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
> > > > +         !(val & SOF_TIMESTAMPING_SOFTWARE))
> > > > +             return -EINVAL;
> > > > +
> > >
> > > This breaks hardware timestamping
> >
> > Yes, and sorry about that. I'll fix this.
>
> As is I don't understand the purpose of this patch. Please do not
> just resubmit with a change, but explain the problem and suggested
> solution first.
>

I will drop this patch because I just tested with my program in the
local machine and found there is one mistake I made about calculating
the diff between those two . Sorry for the noise.

Well, I only need to send a V2 patch of patch [3/3] in the next few days.

BTW, please allow me to ask one question unrelated to this patch
again. I do wonder: if we batch the recv skbs from the errqueue when
calling tcp_recvmsg() -> inet_recv_error(), it could break users,
right?

Thanks,
Jason
diff mbox series

Patch

diff --git a/net/core/sock.c b/net/core/sock.c
index fe87f9bd8f16..4bddd6f62e4f 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -905,6 +905,10 @@  int sock_set_timestamping(struct sock *sk, int optname,
 	if (val & ~SOF_TIMESTAMPING_MASK)
 		return -EINVAL;
 
+	if (val & SOF_TIMESTAMPING_TX_RECORD_MASK &&
+	    !(val & SOF_TIMESTAMPING_SOFTWARE))
+		return -EINVAL;
+
 	if (val & SOF_TIMESTAMPING_OPT_ID_TCP &&
 	    !(val & SOF_TIMESTAMPING_OPT_ID))
 		return -EINVAL;