| 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: <3d71aad8-57db-e8ab-f216-3028206eae38@linux.vnet.ibm.com>
Date: Tue, 7 Jan 2020 11:55:49 -0600
From: Cristobal Forno <cforno12@...ux.vnet.ibm.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.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
Thanks for your suggestions Willlem. I have a question on one of your
suggestions for the ethtool_virtdev_get_link_ksettings function below.
On 22/12/2019 15:19, Willem de Bruijn wrote:
> 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.
Good point, I will incorporate this into the next version of the series.
>> +{
>> + 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.
Agreed.
>
> More fundamentally, these three assignments are simple enough that I
> don't think a helper actually simplifies anything here.
>
Although the function is simple, it does achieve the goal of this
version of the patch series which is to eliminate duplication of code
throughout the virtual devices. I think it's best to leave it like this,
but I am open to more suggestions.
-Cris Forno
>
>> +{
>> + 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