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 10:19:11 -0600
From:   Max Georgiev <glipus@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....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 5, 2023 at 6:26 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Wed, Apr 05, 2023 at 12:33:23AM -0600, Maxim Georgiev wrote:
> > This patch makes VLAN subsystem to use the newly introduced
> > ndo_hwtstamp_get/set API to pass hw timestamp requests to
> > underlying NIC drivers in case if these drivers implement
> > ndo_hwtstamp_get/set functions. Otherwise VLAN┬Ěsubsystem
>
> Strange symbols (┬Ě).

Bad copy-paste, sorry. Fixed.

>
> > falls back to calling ndo_eth_ioctl.
> >
> > Suggested-by: Vladimir Oltean <vladimir.oltean@....com>
> > Signed-off-by: Maxim Georgiev <glipus@...il.com>
> > ---
> >  net/8021q/vlan_dev.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> > index 5920544e93e8..66d54c610aa5 100644
> > --- a/net/8021q/vlan_dev.c
> > +++ b/net/8021q/vlan_dev.c
> > @@ -353,6 +353,44 @@ static int vlan_dev_set_mac_address(struct net_device *dev, void *p)
> >       return 0;
> >  }
> >
> > +static int vlan_dev_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
> > +{
> > +     const struct net_device_ops *ops = dev->netdev_ops;
> > +     struct kernel_hwtstamp_config kernel_config = {};
> > +     struct hwtstamp_config config;
> > +     int err;
> > +
> > +     if (!netif_device_present(dev))
> > +             return -ENODEV;
> > +
> > +     if ((cmd == SIOCSHWTSTAMP && !ops->ndo_hwtstamp_set) ||
> > +         (cmd == SIOCGHWTSTAMP && !ops->ndo_hwtstamp_get)) {
> > +             if (ops->ndo_eth_ioctl) {
> > +                     return ops->ndo_eth_ioctl(real_dev, &ifr, cmd);
> > +             else
> > +                     return -EOPNOTSUPP;
> > +     }
> > +
> > +     kernel_config.ifr = ifr;
> > +     if (cmd == SIOCSHWTSTAMP) {
> > +             if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
> > +                     return -EFAULT;
> > +
> > +             hwtstamp_config_to_kernel(&kernel_config, &config);
> > +             err = ops->ndo_hwtstamp_set(dev, &kernel_config, NULL);
> > +     } else if (cmd == SIOCGHWTSTAMP) {
> > +             err = ops->ndo_hwtstamp_get(dev, &kernel_config, NULL);
> > +     }
> > +
> > +     if (err)
> > +             return err;
> > +
> > +     hwtstamp_kernel_to_config(&config, &kernel_config);
> > +     if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
> > +             return -EFAULT;
> > +     return 0;
> > +}
> > +
> >  static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> >  {
> >       struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
> > @@ -368,10 +406,12 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> >               if (!net_eq(dev_net(dev), dev_net(real_dev)))
> >                       break;
> >               fallthrough;
> > +     case SIOCGHWTSTAMP:
> > +             err = vlan_dev_hwtstamp(real_dev, &ifrr, cmd);
> > +             break;
> >       case SIOCGMIIPHY:
> >       case SIOCGMIIREG:
> >       case SIOCSMIIREG:
> > -     case SIOCGHWTSTAMP:
>
> 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()?

>
> 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.

>
> >               if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
> >                       err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
> >               break;
> > --
> > 2.39.2
> >