From: Andrew Lunn <andrew@lunn•ch>
To: Daniil Tatianin <d-tatianin@yandex-team•ru>
Cc: netdev@vger•kernel.org, Michal Kubecek <mkubecek@suse•cz>,
Jakub Kicinski <kuba@kernel•org>
Subject: Re: [PATCH v2] net/ethtool/ioctl: ensure that we have phy ops before using them
Date: Tue, 22 Nov 2022 18:37:21 +0100 [thread overview]
Message-ID: <Y30I0d6Dd+s9Ak0W@lunn.ch> (raw)
In-Reply-To: <20221122072143.53841-1-d-tatianin@yandex-team.ru>
On Tue, Nov 22, 2022 at 10:21:43AM +0300, Daniil Tatianin wrote:
> ops->get_ethtool_phy_stats was getting called in an else branch
> of ethtool_get_phy_stats() unconditionally without making sure
> it was actually present.
>
> Refactor the checks to avoid unnecessary nesting and make them more
> readable. Add an extra WARN_ON_ONCE(1) to emit a warning when a driver
> declares that it has phy stats without a way to retrieve them.
So i have two different suggestions here, take your pick and even
merge them together.
I wonder if we can simply this some more. If there are 0 stats we
already issue a WARN_ON_ONCE():
https://elixir.bootlin.com/linux/v6.1-rc6/source/net/ethtool/ioctl.c#L2096
We will then copy back to user space the ethtool_stats and zero
statistics and return 0. If that useful? Would it make more sense to
just return -EOPNOTSUPP after the WARN_ON_ONCE()?
That would be patch 1/X.
Patch 2/X would then remove the if (n_stats) code, but otherwise make
no changes. That keeps the patch simple to review.
Patch 3/X would then add the additional verification of
ops->get_ethtool_phy_stats(). But do it at the top of the function,
along with all the other verification, and return -EOPNOTSUPP.
Alternatively, given the complexity of the checking and the function
as a whole, i'm wondering if it make sense to actually pull this
function apart. Add a ethtool_get_phy_stats_phydev() and
ethtool_get_phy_stats_ethtool(), and have ethtool_get_phy_stats() do
the copy_from_user(), call one of the two helpers, and if they don't
return an error do the two copy_to_user().
Andrew
next prev parent reply other threads:[~2022-11-22 17:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-22 7:21 [PATCH v2] net/ethtool/ioctl: ensure that we have phy ops before using them Daniil Tatianin
2022-11-22 17:37 ` Andrew Lunn [this message]
2022-11-23 16:06 ` Alexander Lobakin
[not found] <20221121140556.41763-1-d-tatianin@yandex-team.ru>
2022-11-22 4:29 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y30I0d6Dd+s9Ak0W@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=d-tatianin@yandex-team$(echo .)ru \
--cc=kuba@kernel$(echo .)org \
--cc=mkubecek@suse$(echo .)cz \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox