| 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
| ||
|
Message-ID: <CA+FuTSeJ_3T5K3NrZnqcG6qadOHsoqKRt_ZMPM=fqUqJknDP_Q@mail.gmail.com>
Date: Sun, 22 Dec 2019 16:19:56 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Cris Forno <cforno12@...ux.vnet.ibm.com>
Cc: Network Development <netdev@...r.kernel.org>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>, haiyangz@...rosoft.com,
sthemmin@...rosoft.com, sashal@...nel.org, tlfalcon@...ux.ibm.com
Subject: Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool
link settings for virtual devices to core
On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@...ux.vnet.ibm.com> wrote:
>
> Three virtual devices (ibmveth, virtio_net, and netvsc) all have
> similar code to set/get link settings and validate ethtool command. To
> eliminate duplication of code, it is factored out into core/ethtool.c.
>
> Signed-off-by: Cris Forno <cforno12@...ux.vnet.ibm.com>
> ---
> include/linux/ethtool.h | 2 ++
> net/core/ethtool.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 95991e43..1b0417b 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -394,6 +394,8 @@ struct ethtool_ops {
> struct ethtool_coalesce *);
> int (*set_per_queue_coalesce)(struct net_device *, u32,
> struct ethtool_coalesce *);
> + bool (*virtdev_validate_link_ksettings)(const struct
> + ethtool_link_ksettings *);
> int (*get_link_ksettings)(struct net_device *,
> struct ethtool_link_ksettings *);
> int (*set_link_ksettings)(struct net_device *,
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index cd9bc67..4091a94 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -579,6 +579,32 @@ static int load_link_ksettings_from_user(struct ethtool_link_ksettings *to,
> return 0;
> }
>
> +/* Check if the user is trying to change anything besides speed/duplex */
> +static bool
> +ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd)
If called from other modules like drivers/net/virtio_net.ko, these
functions cannot be static, need a declaration in
include/linux/ethtool.h and an EXPORT_SYMBOL_GPL.
Also return type should be on the same line.
> +{
> + struct ethtool_link_ksettings diff1 = *cmd;
> + struct ethtool_link_ksettings diff2 = {};
> +
> + /* cmd is always set so we need to clear it, validate the port type
> + * and also without autonegotiation we can ignore advertising
> + */
> + diff1.base.speed = 0;
> + diff2.base.port = PORT_OTHER;
> + ethtool_link_ksettings_zero_link_mode(&diff1, advertising);
> + diff1.base.duplex = 0;
> + diff1.base.cmd = 0;
> + diff1.base.link_mode_masks_nwords = 0;
> +
> + return !memcmp(&diff1.base, &diff2.base, sizeof(diff1.base)) &&
> + bitmap_empty(diff1.link_modes.supported,
> + __ETHTOOL_LINK_MODE_MASK_NBITS) &&
> + bitmap_empty(diff1.link_modes.advertising,
> + __ETHTOOL_LINK_MODE_MASK_NBITS) &&
> + bitmap_empty(diff1.link_modes.lp_advertising,
> + __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
> /* convert a kernel internal ethtool_link_ksettings to
> * ethtool_link_usettings in user space. return 0 on success, errno on
> * error.
> @@ -660,6 +686,17 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
> return store_link_ksettings_for_user(useraddr, &link_ksettings);
> }
>
> +static int
> +ethtool_virtdev_get_link_ksettings(struct net_device *dev,
> + struct ethtool_link_ksettings *cmd,
> + u32 *speed, u8 *duplex)
No need to pass by reference, really. Indeed, the virtio_net caller
passes vi->speed and vi->duplex instead of &vi->speed and &vi->duplex.
More fundamentally, these three assignments are simple enough that I
don't think a helper actually simplifies anything here.
> +{
> + cmd->base.speed = *speed;
> + cmd->base.duplex = *duplex;
> + cmd->base.port = PORT_OTHER;
> + return 0;
> +}
> +
> /* Update device ethtool_link_settings. */
> static int ethtool_set_link_ksettings(struct net_device *dev,
> void __user *useraddr)
> @@ -696,6 +733,27 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
> return dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
> }
>
> +static int
> +ethtool_virtdev_set_link_ksettings(struct net_device *dev,
> + const struct ethtool_link_ksettings *cmd,
> + u32 *dev_speed, u8 *dev_duplex)
> +{
> + u32 speed;
> + u8 duplex;
> +
> + speed = cmd->base.speed;
> + duplex = cmd->base.duplex;
> + /* don't allow custom speed and duplex */
> + if (!ethtool_validate_speed(speed) ||
> + !ethtool_validate_duplex(duplex) ||
> + !dev->ethtool_ops->virtdev_validate_link_ksettings(cmd))
> + return -EINVAL;
> + *dev_speed = speed;
> + *dev_duplex = duplex;
> +
> + return 0;
> +}
> +
> /* Query device for its ethtool_cmd settings.
> *
> * Backward compatibility note: for compatibility with legacy ethtool, this is
> --
> 1.8.3.1
>
Powered by blists - more mailing lists