netdev - Re: [PATCHv2 net-next] bonding: add software tx timestamping support

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]

Message-ID: <CANn89iL_7CYs1kAY8SsUJLFoSZXe1rXAd3HJY-9dziehTfTkaQ@mail.gmail.com>
Date:   Fri, 7 Apr 2023 11:34:23 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Toppins <jtoppins@...hat.com>,
        Paolo Abeni <pabeni@...hat.com>, Liang Li <liali@...hat.com>,
        Miroslav Lichvar <mlichvar@...hat.com>
Subject: Re: [PATCHv2 net-next] bonding: add software tx timestamping support

On Fri, Apr 7, 2023 at 8:12 AM Hangbin Liu <liuhangbin@...il.com> wrote:
>
> Currently, bonding only obtain the timestamp (ts) information of
> the active slave, which is available only for modes 1, 5, and 6.
> For other modes, bonding only has software rx timestamping support.
>
> However, some users who use modes such as LACP also want tx timestamp
> support. To address this issue, let's check the ts information of each
> slave. If all slaves support tx timestamping, we can enable tx
> timestamping support for the bond.
>
> Suggested-by: Miroslav Lichvar <mlichvar@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++++++--
>  include/uapi/linux/net_tstamp.h |  3 +++
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 00646aa315c3..994efc777a77 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -5686,9 +5686,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>                                     struct ethtool_ts_info *info)
>  {
>         struct bonding *bond = netdev_priv(bond_dev);
> +       struct ethtool_ts_info ts_info;
>         const struct ethtool_ops *ops;
>         struct net_device *real_dev;
>         struct phy_device *phydev;
> +       bool soft_support = false;
> +       struct list_head *iter;
> +       struct slave *slave;
>         int ret = 0;
>
>         rcu_read_lock();
> @@ -5707,10 +5711,41 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>                         ret = ops->get_ts_info(real_dev, info);
>                         goto out;
>                 }
> +       } else {
> +               /* Check if all slaves support software rx/tx timestamping */
> +               rcu_read_lock();
> +               bond_for_each_slave_rcu(bond, slave, iter) {
> +                       ret = -1;
> +                       dev_hold(slave->dev);

You are holding rcu_read_lock() during the loop, so there is no need
for this dev_hold() / dev_put() dance.

And if this was needed, we kindly ask for new dev_hold()/dev_put()
added in networking code to
instead use netdev_hold / netdev_put(), we have spent enough time
tracking hold/put bugs.

Thanks.


> +                       ops = slave->dev->ethtool_ops;
> +                       phydev = slave->dev->phydev;
> +
> +                       if (phy_has_tsinfo(phydev))
> +                               ret = phy_ts_info(phydev, &ts_info);
> +                       else if (ops->get_ts_info)
> +                               ret = ops->get_ts_info(slave->dev, &ts_info);
> +
> +                       if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_SOFTRXTX) == \
> +                                   SOF_TIMESTAMPING_SOFTRXTX) {
> +                               dev_put(slave->dev);
> +                               soft_support = true;
> +                               continue;
> +                       }
> +
> +                       soft_support = false;
> +                       dev_put(slave->dev);
> +                       break;
> +               }
> +               rcu_read_unlock();