| 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: <87200f46-b04a-2741-dbe7-fa9260adfb79@linux.vnet.ibm.com>
Date: Tue, 7 Jan 2020 11:45:56 -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