[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231011230821.75axavcrjuy5islt@lion.mk-sys.cz>
Date: Thu, 12 Oct 2023 01:08:21 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jonathan Corbet <corbet@....net>,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>, opendmb@...il.com,
justin.chen@...adcom.com
Subject: Re: [PATCH net-next 1/2] ethtool: Introduce WAKE_MDA
On Wed, Oct 11, 2023 at 03:12:39PM -0700, Florian Fainelli wrote:
> Allow Ethernet PHY and MAC drivers with simplified matching logic to be
> waking-up on a custom MAC destination address. This is particularly
> useful for Ethernet PHYs which have limited amounts of buffering but can
> still wake-up on a custom MAC destination address.
>
> When WAKE_MDA is specified, the "sopass" field in the existing struct
> ethtool_wolinfo is re-purposed to hold a custom MAC destination address
> to match against.
>
> Example:
> ethtool -s eth0 wol e mac-da 01:00:5e:00:00:fb
>
> Signed-off-by: Florian Fainelli <florian.fainelli@...adcom.com>
> ---
> Documentation/networking/ethtool-netlink.rst | 7 ++++++-
> include/uapi/linux/ethtool.h | 10 ++++++++--
> include/uapi/linux/ethtool_netlink.h | 1 +
> net/ethtool/common.c | 1 +
> net/ethtool/netlink.h | 2 +-
> net/ethtool/wol.c | 21 ++++++++++++++++++++
> 6 files changed, 38 insertions(+), 4 deletions(-)
>
[...]
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f7fba0dc87e5..8134ac8870bd 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -207,12 +207,17 @@ struct ethtool_drvinfo {
> * @wolopts: Bitmask of %WAKE_* flags for enabled Wake-On-Lan modes.
> * @sopass: SecureOn(tm) password; meaningful only if %WAKE_MAGICSECURE
> * is set in @wolopts.
> + * @mac_da: Destination MAC address to match; meaningful only if
> + * %WAKE_MDA is set in @wolopts.
> */
> struct ethtool_wolinfo {
> __u32 cmd;
> __u32 supported;
> __u32 wolopts;
> - __u8 sopass[SOPASS_MAX];
> + union {
> + __u8 sopass[SOPASS_MAX];
> + __u8 mac_da[ETH_ALEN];
> + };
> };
If we use the union here, we should also make sure the request cannot
set both WAKE_MAGICSECURE and WAKE_MDA, otherwise the same data will be
used for two different values (and interpreted in two different ways in
GET requests and notifications).
Another option would be keeping the existing structure for ioctl UAPI
and using another structure (like we did in other cases where we needed
new attributes beyond frozen ioctl structures) so that a combination of
WAKE_MAGICSECURE and WAKE_MDA is possible. (It doesn't seem to make much
sense to me to combine WAKE_MAGICSECURE with other bits but I can't rule
out someone might want that one day.)
[...]
> diff --git a/net/ethtool/wol.c b/net/ethtool/wol.c
> index 0ed56c9ac1bc..13dfcc9bb1e5 100644
> --- a/net/ethtool/wol.c
> +++ b/net/ethtool/wol.c
> @@ -12,6 +12,7 @@ struct wol_reply_data {
> struct ethnl_reply_data base;
> struct ethtool_wolinfo wol;
> bool show_sopass;
> + bool show_mac_da;
> };
>
> #define WOL_REPDATA(__reply_base) \
> @@ -41,6 +42,8 @@ static int wol_prepare_data(const struct ethnl_req_info *req_base,
> /* do not include password in notifications */
> data->show_sopass = !genl_info_is_ntf(info) &&
> (data->wol.supported & WAKE_MAGICSECURE);
> + data->show_mac_da = !genl_info_is_ntf(info) &&
> + (data->wol.supported & WAKE_MDA);
>
> return 0;
> }
The test for !genl_info_is_ntf(info) above is meant to prevent the
sopass value (which is supposed to be secret) from being included in
notifications which can be seen by unprivileged users. Is the MAC
address to match also supposed to be secret?
Michal
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists