Jason Xing wrote:
> From: Jason Xing <[email protected]>
>
> In the previous patch, we found things could happen in the rx software
> timestamp. Here, we also noticed that, for rx hardware timestamp case,
> it could happen when one process enables the rx hardware timestamp
> generating flag first, then another process only setting
> SOF_TIMESTAMPING_RAW_HARDWARE report flag can still get the hardware
> timestamp.
>
> In this patch, we extend the OPT_RX_FILTER flag to filter out the
> above case for hardware use.
>
> Suggested-by: Jakub Kicinski <[email protected]>
> Signed-off-by: Jason Xing <[email protected]>
> ---
> Link: https://lore.kernel.org/all/[email protected]/
> ---
> Documentation/networking/timestamping.rst | 15 +++++++++------
> net/core/sock.c | 5 +++--
> net/ipv4/tcp.c | 3 ++-
> net/socket.c | 3 ++-
> 4 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/networking/timestamping.rst
> b/Documentation/networking/timestamping.rst
> index ac57d9de2f11..55e79ea71f3e 100644
> --- a/Documentation/networking/timestamping.rst
> +++ b/Documentation/networking/timestamping.rst
> @@ -268,12 +268,15 @@ SOF_TIMESTAMPING_OPT_TX_SWHW:
> each containing just one timestamp.
>
> SOF_TIMESTAMPING_OPT_RX_FILTER:
> - Used in the receive software timestamp. Enabling the flag along with
> - SOF_TIMESTAMPING_SOFTWARE will not report the rx timestamp to the
> - userspace so that it can filter out the case where one process starts
> - first which turns on netstamp_needed_key through setting generation
> - flags like SOF_TIMESTAMPING_RX_SOFTWARE, then another one only passing
> - SOF_TIMESTAMPING_SOFTWARE report flag could also get the rx timestamp.
> + Used in the receive software/hardware timestamp. Enabling the flag
> + along with SOF_TIMESTAMPING_SOFTWARE/SOF_TIMESTAMPING_RAW_HARDWARE
> + will not report the rx timestamp to the userspace so that it can
> + filter out the cases where 1) one process starts first which turns
> + on netstamp_needed_key through setting generation flags like
> + SOF_TIMESTAMPING_RX_SOFTWARE, or 2) similarly one process enables
> + generating the hardware timestamp already, then another one only
> + passing SOF_TIMESTAMPING_SOFTWARE report flag could also get the
> + rx timestamp.
I think this patch should be squashed into patch 1.
Else SOF_TIMESTAMPING_OPT_RX_FILTER has two subtly different behaviors
across its lifetime. Even if it is only two SHA1s apart.
It also avoids such duplicate changes to the same code/text blocks.
More importantly, it matters for the behavior, see below.
>
> SOF_TIMESTAMPING_OPT_RX_FILTER prevents the application from being
> influenced by others and let the application choose whether to report
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 6a93344e21cf..dc4a43cfff59 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -908,8 +908,9 @@ int sock_set_timestamping(struct sock *sk, int optname,
> !(val & SOF_TIMESTAMPING_OPT_ID))
> return -EINVAL;
>
> - if (val & SOF_TIMESTAMPING_RX_SOFTWARE &&
> - val & SOF_TIMESTAMPING_OPT_RX_FILTER)
> + if (val & SOF_TIMESTAMPING_OPT_RX_FILTER &&
> + (val & SOF_TIMESTAMPING_RX_SOFTWARE ||
> + val & SOF_TIMESTAMPING_RX_HARDWARE))
> return -EINVAL;
There may be legitimate use cases of wanting to receive hardware
receive timestamps, plus software transmit timestamp, but
suppress spurious software timestamps (or vice versa):
SOF_TIMESTAMPING_RAW_HARDWARE | \
SOF_TIMESTAMPING_RX_HARDWARE | \
SOF_TIMESTAMPING_SOFTWARE | \
SOF_TIMESTAMPING_TX_SOFTWARE | \
SOF_TIMESTAMPING_OPT_RX_FILTER
Admittedly this seems a bit contrived. But it's little hassle to
support it?
We just can no longer use the branch simplification that Jakub
pointed out.
