netdev - Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get

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:   Thu, 7 Jan 2021 13:58:38 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        George McCollister <george.mccollister@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Arnd Bergmann <arnd@...db.de>, Taehee Yoo <ap420073@...il.com>,
        Jiri Pirko <jiri@...lanox.com>, Florian Westphal <fw@...len.de>
Subject: Re: [PATCH v3 net-next 10/12] net: bonding: ensure .ndo_get_stats64
 can sleep

On Thu, Jan 7, 2021 at 12:33 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Thu, Jan 07, 2021 at 12:18:28PM +0100, Eric Dumazet wrote:
> > What a mess really.
>
> Thanks, that's at least _some_ feedback :)

Yeah, I was on PTO for the last two weeks.

>
> > You chose to keep the assumption that ndo_get_stats() would not fail,
> > since we were providing the needed storage from callers.
> >
> > If ndo_get_stats() are now allowed to sleep, and presumably allocate
> > memory, we need to make sure
> > we report potential errors back to the user.
> >
> > I think your patch series is mostly good, but I would prefer not
> > hiding errors and always report them to user space.
> > And no, netdev_err() is not appropriate, we do not want tools to look
> > at syslog to guess something went wrong.
>
> Well, there are only 22 dev_get_stats callers in the kernel, so I assume
> that after the conversion to return void, I can do another conversion to
> return int, and then I can convert the ndo_get_stats64 method to return
> int too. I will keep the plain ndo_get_stats still void (no reason not
> to).
>
> > Last point about drivers having to go to slow path, talking to
> > firmware : Make sure that malicious/innocent users
> > reading /proc/net/dev from many threads in parallel wont brick these devices.
> >
> > Maybe they implicitly _relied_ on the fact that firmware was gently
> > read every second and results were cached from a work queue or
> > something.
>
> How? I don't understand how I can make sure of that.

Your patches do not attempt to change these drivers, but I guess your
cover letter might send to driver maintainers incentive to get rid of their
logic, that is all.

We might simply warn maintainers and ask them to test their future changes
with tests using 1000 concurrent theads reading /proc/net/dev

>
> There is an effort initiated by Jakub to standardize the ethtool
> statistics. My objection was that you can't expect that to happen unless
> dev_get_stats is sleepable just like ethtool -S is. So I think the same
> reasoning should apply to ethtool -S too, really.

I think we all agree on the principles, once we make sure to not
add more pressure on RTNL. It seems you addressed our feedback, all is fine.

Thanks.