netdev - Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

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:   Wed, 5 Apr 2023 19:28:58 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Max Georgiev <glipus@...il.com>
Cc:     kory.maincent@...tlin.com, kuba@...nel.org, netdev@...r.kernel.org,
        maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev,
        richardcochran@...il.com, gerhard@...leder-embedded.com
Subject: Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code
 path

On Wed, Apr 05, 2023 at 10:19:11AM -0600, Max Georgiev wrote:
> > I would recommend also making vlan_dev_hwtstamp() be called from the
> > VLAN driver's ndo_hwtstamp_set() rather than from ndo_eth_ioctl().
> 
> Vladimir, could you please elaborate here a bit?
> Are you saying that I should go all the way with vlan NDO conversion,
> implement ndo_hwtstamp_get/set() for vlan, and stop handling
> SIOCGHWTSTAMP/SIOCSHWTSTAMP in vlan_dev_ioctl()?

Yes, sorry for being unclear.

> > My understanding of Jakub's suggestion to (temporarily) stuff ifr
> > inside kernel_config was to do that from top-level net/core/dev_ioctl.c,
> > not from the VLAN driver.
> 
> [RFC PATCH v3 2/5] in this patch stack changes net/core/dev_ioctl.c
> to insert ifr inside kernel_config. I assumed that I should do it here too
> so underlying drivers could rely on ifr pointer in kernel_config being
> always initialized.
> If the plan is to stop supporting SIOCGHWTSTAMP/SIOCSHWTSTAMP
> in vlan_dev_ioctl() all together and move the hw timestamp handling
> logic to vlan_get/set_hwtstamp() functions, then this ifr initialization
> code will be removed from net/8021q/vlan_dev.c anyway.

Yes, correct, dev_set_hwtstamp() should provide it.

There's a small thing I don't like about stuffing "ifr" inside struct
kernel_hwtstamp_config, and that's that some drivers keep the last
configuration privately using memcpy(). If we put "ifr" there, they will
practically have access to a stale pointer, because "ifr" loses meaning
once the ioctl syscall is over.

Since we don't know how long it will take until the ndo_hwtstamp_set()
conversion is complete (experience says: possibly indefinitely), it
would be good if this wasn't possible, because who knows what ideas
people might get to do with it.

Options to avoid it are:
- keep doing what you're doing - let drivers memcpy() the struct
  hwtstamp_config and not the struct kernel_hwtstamp_config.
- pass the ifr as yet another argument to ndo_hwtstamp_set(), and don't
  stuff it inside struct kernel_hwtstamp_config.

Neither of these is particularly great, because at the end of the
conversion, some extra cleanup will be required to fix up the API again
(either to stop all drivers from using struct hwtstamp_config, or to
stop passing the argument which is no longer used by anybody).

I think, personally, I'd opt for the first bullet, keep doing what
you're doing. It should require a bit less cleanup, since not all
drivers do the memcpy() thing.