netdev - Re: [PATCH 3/8] socket: Disentangle SOCK_RCVTSTAMPNS from SOCK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Nov 2018 18:31:32 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Deepa Dinamani <deepa.kernel@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        y2038 Mailman List <y2038@...ts.linaro.org>
Subject: Re: [PATCH 3/8] socket: Disentangle SOCK_RCVTSTAMPNS from SOCK_RCVTSTAMP

On Fri, Nov 30, 2018 at 5:16 PM Deepa Dinamani <deepa.kernel@...il.com> wrote:
>
> On Sun, Nov 25, 2018 at 10:19 AM David Miller <davem@...emloft.net> wrote:
> >
> > From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> > Date: Sun, 25 Nov 2018 09:18:55 -0500
> >
> > > The existing logic is as is for a reason. There is no need to change
> > > it to satisfy the main purpose of your patchset?
> > >
> > > It is structured as one bit to test whether a timestamp is requested
> > > and another to select among two variants usec/nsec. Just add another
> > > layer of branching between new/old in cases where this distinction is
> > > needed.
> > >
> > > Please avoid code churn unless needed.
> >
> > +1
>
> This patch makes it easier to add logic for 2 new socket time options.
> But, if you prefer for all of the options to depend on SOCK_RCVTSTAMP
> then I will drop it.

Yes, please keep as is.

I don't see how this change is needed to significantly simplify the
main patchset, and an unnecessary change can cause an unforeseen
regression (as was the case with doubling the tests in the hot path).

The current approach has one branch in the hot path where timestamps
are disabled and then selects from two variants where it is enabled:

    if (rcvtstamp) {
      if (rcvtstamp_ns)
        ..
      else
        ..
    }

Both of these need to be split into new and old variants. The way to
achieve that with minimal code perturbation is

    if (rcvtstamp) {
  +    if (sk_timestamping_new)
  +     return __sock_recv_timestamp_new(..)
  +
      if (rcvtstamp_ns)
        ..
      else
        ..
   }

Or alternatively add a check for new in each of the inner branches. In
any case, please be consistent between sock_recv_sw_timestamp and
tcp_recv_sw_timestamp. The current patchset alternates them.