Message-ID: <20230406150157.rwpmghgao77lkdny@skbuf>
Date:   Thu, 6 Apr 2023 18:01:57 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Max Georgiev <glipus@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, kory.maincent@...tlin.com,
        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 Thu, Apr 06, 2023 at 12:21:37AM -0600, Max Georgiev wrote:
> I tried my best to follow the discussion, and convert it to compilable code.
> Here is what I have in mind for generic_hwtstamp_get_lower():
> 
> int generic_hwtstamp_get_lower(struct net_dev *dev,
>                            struct kernel_hwtstamp_config *kernel_cfg,
>                            struct netlink_ext_ack *extack)
> {
>         const struct net_device_ops *ops = dev->netdev_ops;
>         struct hwtstamp_config cfg;
>         int err;
> 
>         if (!netif_device_present(dev))
>                 return -ENODEV;
> 
>         if (ops->ndo_hwtstamp_get)
>                 return ops->ndo_hwtstamp_get(dev, cfg, extack);
> 
>         if (!cfg->ifr)
>                 return -EOPNOTSUPP;
> 
>         err = dev_eth_ioctl(dev, cfg->ifr, SIOCGHWTSTAMP);
>         if (err)
>             return err;
> 
>         if (copy_from_user(&cfg, cfg->ifr->ifr_data, sizeof(cfg)))
>                 return -EFAULT;
> 
>         hwtstamp_config_to_kernel(kernel_cfg, &cfg);
> 
>         return 0;
> }

Side note, it doesn't look like this code is particularly compilable
either - "cfg" is used in some places instead of "kernel_cfg".

> 
> It looks like there is a possibility that the returned hwtstamp_config structure
> will be copied twice to ifr and copied once from ifr on the return path
> in case if the underlying driver does not implement ndo_hwtstamp_get():
> - the underlying driver calls copy_to_user() inside its ndo_eth_ioctl()
>   implementation to return the data to generic_hwtstamp_get_lower();
> - then generic_hwtstamp_get_lower() calls copy_from_user() to copy it
>   back out of the ifr to kernel_hwtstamp_config structure;
> - then dev_get_hwtstamp() calls copy_to_user() again to update
>   the same ifr with the same data the ifr already contains.
> 
> Should we consider this acceptable?

Thanks for laying this out. I guess with a table it's going to be
clearer, so to summarize, I believe this is the status:

Assuming we convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
driver uses ndo_eth_ioctl(), we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the real_dev's ndo_eth_ioctl()
- one copy_to_user() in the real_dev's ndo_eth_ioctl()
- one copy_from_user() in generic_hwtstamp_get_lower()
- one copy_to_user() in dev_set_hwtstamp()

If the vlan driver is converted to ndo_hwtstamp_set() and the real_dev
is converted too, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_to_user() in dev_set_hwtstamp()

===================

Assuming we don't convert *vlan to ndo_hwtstamp_set():

===================

If the vlan driver isn't converted to ndo_hwtstamp_set() and the
real_dev driver isn't converted either, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the real_dev's ndo_eth_ioctl()
- one copy_to_user() in the real_dev's ndo_eth_ioctl()

If the vlan driver isn't converted to ndo_hwtstamp_set(), but the
real_dev driver is, we have:
- one copy_from_user() in dev_set_hwtstamp()
- one copy_from_user() in the vlan's ndo_eth_ioctl()
- one copy_to_user() in the vlan's ndo_eth_ioctl()

===================

So between converting and not converting the *vlans to ndo_hwtstamp_set(),
the worst case is going to be worse (with a mix of new API in *vlan and
old API in real_dev) and the best case is going to be better (with new
API in both *vlan and real_dev). OTOH, with old API in *vlan, the number
of copies to/from the user buffer is going to be constant at 3, which is
not the best, not the worst.

I guess the data indicates that we should convert the *vlans to
ndo_hwtstamp_set() at the very end of the process, and for now, just
make them compatible with a real_dev that uses the new API?

Note that I haven't done the math for the "get" operation yet, but I
believe it to be similar.

Powered by blists - more mailing lists