Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Vladimir Oltean]

Hello Willem,

I have an application which makes use of SOF_TIMESTAMPING_OPT_ID, and I
received reports from multiple users that all timestamps are delivered
with a tskey of 0 for all stable kernel branches earlier than, and
including, 4.19.

I bisected this issue down to:

| commit 8f932f762e7928d250e21006b00ff9b7718b0a64 (HEAD)
| Author: Willem de Bruijn <willemb@google.com>
| Date:   Mon Dec 17 12:24:00 2018 -0500
| 
|     net: add missing SOF_TIMESTAMPING_OPT_ID support
| 
|     SOF_TIMESTAMPING_OPT_ID is supported on TCP, UDP and RAW sockets.
|     But it was missing on RAW with IPPROTO_IP, PF_PACKET and CAN.
| 
|     Add skb_setup_tx_timestamp that configures both tx_flags and tskey
|     for these paths that do not need corking or use bytestream keys.
| 
|     Fixes: 09c2d251b707 ("net-timestamp: add key to disambiguate concurrent datagrams")
|     Signed-off-by: Willem de Bruijn <willemb@google.com>
|     Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
|     Signed-off-by: David S. Miller <davem@davemloft.net>

and, interestingly, I found this discussion on the topic:
https://www.spinics.net/lists/netdev/msg540752.html
(copied here in case the link rots in the future)

| > Series applied.
| >
| > What is your opinion about -stable for this?
| 
| Thanks David. Since these are just missing features that no one has
| reported as actually having been missing a whole lot, I don't think
| that they are worth the effort or risk.

So I have 2 questions:

Is there a way for user space to validate functional kernel support for
SOF_TIMESTAMPING_OPT_ID? What I'm noticing is that (at least with
AF_PACKET sockets) the "level == SOL_PACKET && type == PACKET_TX_TIMESTAMP"
cmsg is _not_ missing, but instead contains a valid sock_err->ee_data
(tskey) of 0.

If it's not possible, could you please consider sending these fixes as
patches to linux-stable?

Re: Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Willem de Bruijn]

"

On Thu, Mar 24, 2022 at 5:43 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hello Willem,
>
> I have an application which makes use of SOF_TIMESTAMPING_OPT_ID, and I
> received reports from multiple users that all timestamps are delivered
> with a tskey of 0 for all stable kernel branches earlier than, and
> including, 4.19.
>
> I bisected this issue down to:
>
> | commit 8f932f762e7928d250e21006b00ff9b7718b0a64 (HEAD)
> | Author: Willem de Bruijn <willemb@google.com>
> | Date:   Mon Dec 17 12:24:00 2018 -0500
> |
> |     net: add missing SOF_TIMESTAMPING_OPT_ID support
> |
> |     SOF_TIMESTAMPING_OPT_ID is supported on TCP, UDP and RAW sockets.
> |     But it was missing on RAW with IPPROTO_IP, PF_PACKET and CAN.
> |
> |     Add skb_setup_tx_timestamp that configures both tx_flags and tskey
> |     for these paths that do not need corking or use bytestream keys.
> |
> |     Fixes: 09c2d251b707 ("net-timestamp: add key to disambiguate concurrent datagrams")
> |     Signed-off-by: Willem de Bruijn <willemb@google.com>
> |     Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> |     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> and, interestingly, I found this discussion on the topic:
> https://www.spinics.net/lists/netdev/msg540752.html
> (copied here in case the link rots in the future)
>
> | > Series applied.
> | >
> | > What is your opinion about -stable for this?
> |
> | Thanks David. Since these are just missing features that no one has
> | reported as actually having been missing a whole lot, I don't think
> | that they are worth the effort or risk.
>
> So I have 2 questions:
>
> Is there a way for user space to validate functional kernel support for
> SOF_TIMESTAMPING_OPT_ID? What I'm noticing is that (at least with
> AF_PACKET sockets) the "level == SOL_PACKET && type == PACKET_TX_TIMESTAMP"
> cmsg is _not_ missing, but instead contains a valid sock_err->ee_data
> (tskey) of 0.

The commit only fixes missing OPT_ID support for PF_PACKET and various SOCK_RAW.

The cmsg structure returned for timestamps is the same regardless of
whether the option is set configured. It just uses an otherwise constant field.

On these kernels the feature is supported, and should work on TCP and UDP.
So a feature check would give the wrong answer.

> If it's not possible, could you please consider sending these fixes as
> patches to linux-stable?

The first of the two fixes

    fbfb2321e9509 ("ipv6: add missing tx timestamping on IPPROTO_RAW")

is in 4.19.y as of 4.19.99

The follow-on fix that you want

    8f932f762e79 ("net: add missing SOF_TIMESTAMPING_OPT_ID support")

applies cleanly to 4.19.236.

I think it's fine to cherry-pick. Not sure how to go about that.

Re: Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Vladimir Oltean]

Hi Willem,

Thanks for the reply.

On Fri, Mar 25, 2022 at 09:15:30AM -0400, Willem de Bruijn wrote:
> On Thu, Mar 24, 2022 at 5:43 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hello Willem,
> >
> > I have an application which makes use of SOF_TIMESTAMPING_OPT_ID, and I
> > received reports from multiple users that all timestamps are delivered
> > with a tskey of 0 for all stable kernel branches earlier than, and
> > including, 4.19.
> >
> > I bisected this issue down to:
> >
> > | commit 8f932f762e7928d250e21006b00ff9b7718b0a64 (HEAD)
> > | Author: Willem de Bruijn <willemb@google.com>
> > | Date:   Mon Dec 17 12:24:00 2018 -0500
> > |
> > |     net: add missing SOF_TIMESTAMPING_OPT_ID support
> > |
> > |     SOF_TIMESTAMPING_OPT_ID is supported on TCP, UDP and RAW sockets.
> > |     But it was missing on RAW with IPPROTO_IP, PF_PACKET and CAN.
> > |
> > |     Add skb_setup_tx_timestamp that configures both tx_flags and tskey
> > |     for these paths that do not need corking or use bytestream keys.
> > |
> > |     Fixes: 09c2d251b707 ("net-timestamp: add key to disambiguate concurrent datagrams")
> > |     Signed-off-by: Willem de Bruijn <willemb@google.com>
> > |     Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> > |     Signed-off-by: David S. Miller <davem@davemloft.net>
> >
> > and, interestingly, I found this discussion on the topic:
> > https://www.spinics.net/lists/netdev/msg540752.html
> > (copied here in case the link rots in the future)
> >
> > | > Series applied.
> > | >
> > | > What is your opinion about -stable for this?
> > |
> > | Thanks David. Since these are just missing features that no one has
> > | reported as actually having been missing a whole lot, I don't think
> > | that they are worth the effort or risk.
> >
> > So I have 2 questions:
> >
> > Is there a way for user space to validate functional kernel support for
> > SOF_TIMESTAMPING_OPT_ID? What I'm noticing is that (at least with
> > AF_PACKET sockets) the "level == SOL_PACKET && type == PACKET_TX_TIMESTAMP"
> > cmsg is _not_ missing, but instead contains a valid sock_err->ee_data
> > (tskey) of 0.
> 
> The commit only fixes missing OPT_ID support for PF_PACKET and various SOCK_RAW.
> 
> The cmsg structure returned for timestamps is the same regardless of
> whether the option is set configured. It just uses an otherwise constant field.
> 
> On these kernels the feature is supported, and should work on TCP and UDP.
> So a feature check would give the wrong answer.

Ok, I read this as "user space can't detect whether OPT_ID works on PF_PACKET sockets",
except by retroactively looking at the tskeys, and if they're all zero, say
"hmm, something's not right". Pretty complicated.

So we probably need to fix the stable kernels. For the particular case
of my application, I have just about zero control of what kernel the
users are running, so the more stable branches we could cover, the better.

> > If it's not possible, could you please consider sending these fixes as
> > patches to linux-stable?
> 
> The first of the two fixes
> 
>     fbfb2321e9509 ("ipv6: add missing tx timestamping on IPPROTO_RAW")
> 
> is in 4.19.y as of 4.19.99
> 
> The follow-on fix that you want
> 
>     8f932f762e79 ("net: add missing SOF_TIMESTAMPING_OPT_ID support")
> 
> applies cleanly to 4.19.236.
> 
> I think it's fine to cherry-pick. Not sure how to go about that.

Do you have any particular concerns about sending this patch to the
linux-stable branches for 4.19, 4.14 and 4.9? From https://www.kernel.org/
I see those are the only stable branches left.

Re: Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Willem de Bruijn]

On Fri, Mar 25, 2022 at 9:37 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Willem,
>
> Thanks for the reply.
>
> On Fri, Mar 25, 2022 at 09:15:30AM -0400, Willem de Bruijn wrote:
> > On Thu, Mar 24, 2022 at 5:43 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Hello Willem,
> > >
> > > I have an application which makes use of SOF_TIMESTAMPING_OPT_ID, and I
> > > received reports from multiple users that all timestamps are delivered
> > > with a tskey of 0 for all stable kernel branches earlier than, and
> > > including, 4.19.
> > >
> > > I bisected this issue down to:
> > >
> > > | commit 8f932f762e7928d250e21006b00ff9b7718b0a64 (HEAD)
> > > | Author: Willem de Bruijn <willemb@google.com>
> > > | Date:   Mon Dec 17 12:24:00 2018 -0500
> > > |
> > > |     net: add missing SOF_TIMESTAMPING_OPT_ID support
> > > |
> > > |     SOF_TIMESTAMPING_OPT_ID is supported on TCP, UDP and RAW sockets.
> > > |     But it was missing on RAW with IPPROTO_IP, PF_PACKET and CAN.
> > > |
> > > |     Add skb_setup_tx_timestamp that configures both tx_flags and tskey
> > > |     for these paths that do not need corking or use bytestream keys.
> > > |
> > > |     Fixes: 09c2d251b707 ("net-timestamp: add key to disambiguate concurrent datagrams")
> > > |     Signed-off-by: Willem de Bruijn <willemb@google.com>
> > > |     Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> > > |     Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > and, interestingly, I found this discussion on the topic:
> > > https://www.spinics.net/lists/netdev/msg540752.html
> > > (copied here in case the link rots in the future)
> > >
> > > | > Series applied.
> > > | >
> > > | > What is your opinion about -stable for this?
> > > |
> > > | Thanks David. Since these are just missing features that no one has
> > > | reported as actually having been missing a whole lot, I don't think
> > > | that they are worth the effort or risk.
> > >
> > > So I have 2 questions:
> > >
> > > Is there a way for user space to validate functional kernel support for
> > > SOF_TIMESTAMPING_OPT_ID? What I'm noticing is that (at least with
> > > AF_PACKET sockets) the "level == SOL_PACKET && type == PACKET_TX_TIMESTAMP"
> > > cmsg is _not_ missing, but instead contains a valid sock_err->ee_data
> > > (tskey) of 0.
> >
> > The commit only fixes missing OPT_ID support for PF_PACKET and various SOCK_RAW.
> >
> > The cmsg structure returned for timestamps is the same regardless of
> > whether the option is set configured. It just uses an otherwise constant field.
> >
> > On these kernels the feature is supported, and should work on TCP and UDP.
> > So a feature check would give the wrong answer.
>
> Ok, I read this as "user space can't detect whether OPT_ID works on PF_PACKET sockets",
> except by retroactively looking at the tskeys, and if they're all zero, say
> "hmm, something's not right". Pretty complicated.
>
> So we probably need to fix the stable kernels. For the particular case
> of my application, I have just about zero control of what kernel the
> users are running, so the more stable branches we could cover, the better.
>
> > > If it's not possible, could you please consider sending these fixes as
> > > patches to linux-stable?
> >
> > The first of the two fixes
> >
> >     fbfb2321e9509 ("ipv6: add missing tx timestamping on IPPROTO_RAW")
> >
> > is in 4.19.y as of 4.19.99
> >
> > The follow-on fix that you want
> >
> >     8f932f762e79 ("net: add missing SOF_TIMESTAMPING_OPT_ID support")
> >
> > applies cleanly to 4.19.236.
> >
> > I think it's fine to cherry-pick. Not sure how to go about that.
>
> Do you have any particular concerns about sending this patch to the
> linux-stable branches for 4.19, 4.14 and 4.9? From https://www.kernel.org/
> I see those are the only stable branches left.

The second patch does not apply cleanly to 4.14.y and even the first
(one-liner) has a conflict on 4.9.y.

It would be good to verify by running the expanded
tools/testing/selftests/net/txtimestamp.c against the patched kernels
first. That should serve as a good test whether the feature works on a
kernel, re: that previous point.

If you want to test and send the 4.19.y patch, please go ahead. Or I
can do it, but it will take some time.

Re: Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Vladimir Oltean]

On Fri, Mar 25, 2022 at 09:48:41AM -0400, Willem de Bruijn wrote:
> > Do you have any particular concerns about sending this patch to the
> > linux-stable branches for 4.19, 4.14 and 4.9? From https://www.kernel.org/
> > I see those are the only stable branches left.
> 
> The second patch does not apply cleanly to 4.14.y and even the first
> (one-liner) has a conflict on 4.9.y.
> 
> It would be good to verify by running the expanded
> tools/testing/selftests/net/txtimestamp.c against the patched kernels
> first. That should serve as a good test whether the feature works on a
> kernel, re: that previous point.
> 
> If you want to test and send the 4.19.y patch, please go ahead. Or I
> can do it, but it will take some time.

I think I do have a setup where I can test all 3 stable kernels.
I'll see if I can backport the SO_TIMESTAMPING fixes to them and
validate using the kernel selftest and my app. If I'm successful,
I'll attach the patchsets here for you to review, then send to stable if
you're okay, would that work?

Re: Broken SOF_TIMESTAMPING_OPT_ID in linux-4.19.y and earlier stable branches

[Willem de Bruijn]

On Fri, Mar 25, 2022 at 10:07 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Fri, Mar 25, 2022 at 09:48:41AM -0400, Willem de Bruijn wrote:
> > > Do you have any particular concerns about sending this patch to the
> > > linux-stable branches for 4.19, 4.14 and 4.9? From https://www.kernel.org/
> > > I see those are the only stable branches left.
> >
> > The second patch does not apply cleanly to 4.14.y and even the first
> > (one-liner) has a conflict on 4.9.y.
> >
> > It would be good to verify by running the expanded
> > tools/testing/selftests/net/txtimestamp.c against the patched kernels
> > first. That should serve as a good test whether the feature works on a
> > kernel, re: that previous point.
> >
> > If you want to test and send the 4.19.y patch, please go ahead. Or I
> > can do it, but it will take some time.
>
> I think I do have a setup where I can test all 3 stable kernels.
> I'll see if I can backport the SO_TIMESTAMPING fixes to them and
> validate using the kernel selftest and my app. If I'm successful,
> I'll attach the patchsets here for you to review, then send to stable if
> you're okay, would that work?

Awesome, thanks. Please don't send them as attachments to the list.
Standard text patches preferably. Off-list is fine too.