diff mbox series

[net,v2,1/2] socket: fix option SO_TIMESTAMPING_NEW

Message ID 20201012093542.15504-1-ceggers@arri.de (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [net,v2,1/2] socket: fix option SO_TIMESTAMPING_NEW | expand

Commit Message

Christian Eggers Oct. 12, 2020, 9:35 a.m. UTC
The comparison of optname with SO_TIMESTAMPING_NEW is wrong way around,
so SOCK_TSTAMP_NEW will first be set and than reset again. Additionally
move it out of the test for SOF_TIMESTAMPING_RX_SOFTWARE as this seems
unrelated.

This problem happens on 32 bit platforms were the libc has already
switched to struct timespec64 (from SO_TIMExxx_OLD to SO_TIMExxx_NEW
socket options). ptp4l complains with "missing timestamp on transmitted
peer delay request" because the wrong format is received (and
discarded).

Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW")
Signed-off-by: Christian Eggers <ceggers@arri.de>
Reviewed-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Reviewed-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
v2:
-----
- integrated proposal from Willem de Bruijn
- added Reviewed-by: from Willem and Deepa


On Saturday, 10 October 2020, 02:23:10 CEST, Willem de Bruijn wrote:
> This suggested fix still sets and clears the flag if calling
> SO_TIMESTAMPING_NEW to disable timestamping. 
where is it cleared?

> Instead, how about
> 
>         case SO_TIMESTAMPING_NEW:
> -               sock_set_flag(sk, SOCK_TSTAMP_NEW);
>                 fallthrough;
>         case SO_TIMESTAMPING_OLD:
> [..]
> +               sock_valbool_flag(sk, SOCK_TSTAMP_NEW,
> +                                 optname == SO_TIMESTAMPING_NEW);
> +
using you version looks clearer

>                 if (val & SOF_TIMESTAMPING_OPT_ID &&
> 
I would like to keep this below the "ret = -FOO; break" statements. IMHO the
setsockopt() call should either completely fail or succeed.

 net/core/sock.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Willem de Bruijn Oct. 12, 2020, 1:37 p.m. UTC | #1
On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> wrote:
>
> The comparison of optname with SO_TIMESTAMPING_NEW is wrong way around,
> so SOCK_TSTAMP_NEW will first be set and than reset again. Additionally
> move it out of the test for SOF_TIMESTAMPING_RX_SOFTWARE as this seems
> unrelated.
>
> This problem happens on 32 bit platforms were the libc has already
> switched to struct timespec64 (from SO_TIMExxx_OLD to SO_TIMExxx_NEW
> socket options). ptp4l complains with "missing timestamp on transmitted
> peer delay request" because the wrong format is received (and
> discarded).
>
> Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW")
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Reviewed-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Reviewed-by: Deepa Dinamani <deepa.kernel@gmail.com>

We have not yet reviewed this second patch. Please do not add such
tags on behalf of other people.

That said, I now have and do agree with the change, so

Acked-by: Willem de Bruijn <willemb@google.com>

> ---
> v2:
> -----
> - integrated proposal from Willem de Bruijn
> - added Reviewed-by: from Willem and Deepa
>
>
> On Saturday, 10 October 2020, 02:23:10 CEST, Willem de Bruijn wrote:
> > This suggested fix still sets and clears the flag if calling
> > SO_TIMESTAMPING_NEW to disable timestamping.
> where is it cleared?
>
> > Instead, how about
> >
> >         case SO_TIMESTAMPING_NEW:
> > -               sock_set_flag(sk, SOCK_TSTAMP_NEW);
> >                 fallthrough;
> >         case SO_TIMESTAMPING_OLD:
> > [..]
> > +               sock_valbool_flag(sk, SOCK_TSTAMP_NEW,
> > +                                 optname == SO_TIMESTAMPING_NEW);
> > +
> using you version looks clearer
>
> >                 if (val & SOF_TIMESTAMPING_OPT_ID &&
> >
> I would like to keep this below the "ret = -FOO; break" statements. IMHO the
> setsockopt() call should either completely fail or succeed.

Agreed.


>  net/core/sock.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 34a8d12e38d7..669cf9b8bb44 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -994,8 +994,6 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
>                 __sock_set_timestamps(sk, valbool, true, true);
>                 break;
>         case SO_TIMESTAMPING_NEW:
> -               sock_set_flag(sk, SOCK_TSTAMP_NEW);
> -               fallthrough;
>         case SO_TIMESTAMPING_OLD:
>                 if (val & ~SOF_TIMESTAMPING_MASK) {
>                         ret = -EINVAL;
> @@ -1024,16 +1022,14 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
>                 }
>
>                 sk->sk_tsflags = val;
> +               sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname == SO_TIMESTAMPING_NEW);
> +
>                 if (val & SOF_TIMESTAMPING_RX_SOFTWARE)
>                         sock_enable_timestamp(sk,
>                                               SOCK_TIMESTAMPING_RX_SOFTWARE);
> -               else {
> -                       if (optname == SO_TIMESTAMPING_NEW)
> -                               sock_reset_flag(sk, SOCK_TSTAMP_NEW);
> -
> +               else
>                         sock_disable_timestamp(sk,
>                                                (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE));
> -               }
>                 break;
>
>         case SO_RCVLOWAT:
> --
> Christian Eggers
> Embedded software developer
>
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
> Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
> Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler
>
Deepa Dinamani Oct. 13, 2020, 5:10 a.m. UTC | #2
> On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> wrote:
> > v2:
> > -----
> > - integrated proposal from Willem de Bruijn
> > - added Reviewed-by: from Willem and Deepa

You may add my
Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>


-Deepa
Jakub Kicinski Oct. 14, 2020, 12:22 a.m. UTC | #3
On Mon, 12 Oct 2020 22:10:31 -0700 Deepa Dinamani wrote:
> > On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> wrote:  
> > > v2:
> > > -----
> > > - integrated proposal from Willem de Bruijn
> > > - added Reviewed-by: from Willem and Deepa  
> 
> You may add my
> Acked-by: Deepa Dinamani <deepa.kernel@gmail.com>

Applied both, thanks everyone!
diff mbox series

Patch

diff --git a/net/core/sock.c b/net/core/sock.c
index 34a8d12e38d7..669cf9b8bb44 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -994,8 +994,6 @@  int sock_setsockopt(struct socket *sock, int level, int optname,
 		__sock_set_timestamps(sk, valbool, true, true);
 		break;
 	case SO_TIMESTAMPING_NEW:
-		sock_set_flag(sk, SOCK_TSTAMP_NEW);
-		fallthrough;
 	case SO_TIMESTAMPING_OLD:
 		if (val & ~SOF_TIMESTAMPING_MASK) {
 			ret = -EINVAL;
@@ -1024,16 +1022,14 @@  int sock_setsockopt(struct socket *sock, int level, int optname,
 		}
 
 		sk->sk_tsflags = val;
+		sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname == SO_TIMESTAMPING_NEW);
+
 		if (val & SOF_TIMESTAMPING_RX_SOFTWARE)
 			sock_enable_timestamp(sk,
 					      SOCK_TIMESTAMPING_RX_SOFTWARE);
-		else {
-			if (optname == SO_TIMESTAMPING_NEW)
-				sock_reset_flag(sk, SOCK_TSTAMP_NEW);
-
+		else
 			sock_disable_timestamp(sk,
 					       (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE));
-		}
 		break;
 
 	case SO_RCVLOWAT: