| Message ID | 20200225214111.4135-2-cforno12@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net/ethtool: Introduce link_ksettings API for virtual network devices | expand |
On Tue, Feb 25, 2020 at 03:41:10PM -0600, Cris Forno wrote: > Three virtual devices (ibmveth, virtio_net, and netvsc) all have > similar code to 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@linux.vnet.ibm.com> > --- [...] > +int ethtool_virtdev_set_link_ksettings(struct net_device *dev, > + const struct ethtool_link_ksettings *cmd, > + u32 *dev_speed, u8 *dev_duplex, > + bool (*dev_virtdev_validate_cmd) > + (const struct ethtool_link_ksettings *)) > +{ > + bool (*validate)(const struct ethtool_link_ksettings *); > + u32 speed; > + u8 duplex; > + > + validate = dev_virtdev_validate_cmd ?: ethtool_virtdev_validate_cmd; > + speed = cmd->base.speed; > + duplex = cmd->base.duplex; > + /* don't allow custom speed and duplex */ > + if (!ethtool_validate_speed(speed) || > + !ethtool_validate_duplex(duplex) || > + !(*validate)(cmd)) > + return -EINVAL; > + *dev_speed = speed; > + *dev_duplex = duplex; > + > + return 0; > +} > +EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings); I didn't realize it when I asked about netvsc_validate_ethtool_ss_cmd() while reviewing v5 but after you got rid of it, all three callers of ethtool_virtdev_set_link_ksettings() call it with NULL as validator, i.e. use the default ethtool_virtdev_validate_cmd(). This brings a question if we really need the possibility to provide a custom validator function. Do you think we should expect some driver needing a custom validator function soon? If not, we should probably use the default validation unconditionally for now and only add the option to provide custom validator function when (if) there is use for it. Michal
Michal Kubecek <mkubecek@suse.cz> writes: > On Tue, Feb 25, 2020 at 03:41:10PM -0600, Cris Forno wrote: >> Three virtual devices (ibmveth, virtio_net, and netvsc) all have >> similar code to 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@linux.vnet.ibm.com> >> --- > [...] >> +int ethtool_virtdev_set_link_ksettings(struct net_device *dev, >> + const struct ethtool_link_ksettings *cmd, >> + u32 *dev_speed, u8 *dev_duplex, >> + bool (*dev_virtdev_validate_cmd) >> + (const struct ethtool_link_ksettings *)) >> +{ >> + bool (*validate)(const struct ethtool_link_ksettings *); >> + u32 speed; >> + u8 duplex; >> + >> + validate = dev_virtdev_validate_cmd ?: ethtool_virtdev_validate_cmd; >> + speed = cmd->base.speed; >> + duplex = cmd->base.duplex; >> + /* don't allow custom speed and duplex */ >> + if (!ethtool_validate_speed(speed) || >> + !ethtool_validate_duplex(duplex) || >> + !(*validate)(cmd)) >> + return -EINVAL; >> + *dev_speed = speed; >> + *dev_duplex = duplex; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings); > > I didn't realize it when I asked about netvsc_validate_ethtool_ss_cmd() > while reviewing v5 but after you got rid of it, all three callers of > ethtool_virtdev_set_link_ksettings() call it with NULL as validator, > i.e. use the default ethtool_virtdev_validate_cmd(). > > This brings a question if we really need the possibility to provide > a custom validator function. Do you think we should expect some driver > needing a custom validator function soon? If not, we should probably use > the default validation unconditionally for now and only add the option > to provide custom validator function when (if) there is use for it. > > Michal I agree, we do not need the custom validator function pointer parameter in the set_link_ksettings function since it is currently not needed. The parameter will be removed in v7. Thanks again Michal! --Cris Forno
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 95991e4300bf..e12a29502e18 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -420,4 +420,12 @@ struct ethtool_rx_flow_rule * ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input); void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *rule); +bool ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd); +int ethtool_virtdev_set_link_ksettings(struct net_device *dev, + const struct ethtool_link_ksettings *cmd, + u32 *dev_speed, u8 *dev_duplex, + bool (*dev_virtdev_validate_cmd) + (const struct ethtool_link_ksettings *)); + + #endif /* _LINUX_ETHTOOL_H */ diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index b987052d91ef..af75a4c0f87c 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -459,6 +459,25 @@ 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 */ +bool ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd) +{ + struct ethtool_link_settings base2 = {}; + + base2.speed = cmd->base.speed; + base2.port = PORT_OTHER; + base2.duplex = cmd->base.duplex; + base2.cmd = cmd->base.cmd; + base2.link_mode_masks_nwords = cmd->base.link_mode_masks_nwords; + + return !memcmp(&base2, &cmd->base, sizeof(base2)) && + bitmap_empty(cmd->link_modes.supported, + __ETHTOOL_LINK_MODE_MASK_NBITS) && + bitmap_empty(cmd->link_modes.lp_advertising, + __ETHTOOL_LINK_MODE_MASK_NBITS); +} +EXPORT_SYMBOL(ethtool_virtdev_validate_cmd); + /* convert a kernel internal ethtool_link_ksettings to * ethtool_link_usettings in user space. return 0 on success, errno on * error. @@ -581,6 +600,31 @@ static int ethtool_set_link_ksettings(struct net_device *dev, return err; } +int ethtool_virtdev_set_link_ksettings(struct net_device *dev, + const struct ethtool_link_ksettings *cmd, + u32 *dev_speed, u8 *dev_duplex, + bool (*dev_virtdev_validate_cmd) + (const struct ethtool_link_ksettings *)) +{ + bool (*validate)(const struct ethtool_link_ksettings *); + u32 speed; + u8 duplex; + + validate = dev_virtdev_validate_cmd ?: ethtool_virtdev_validate_cmd; + speed = cmd->base.speed; + duplex = cmd->base.duplex; + /* don't allow custom speed and duplex */ + if (!ethtool_validate_speed(speed) || + !ethtool_validate_duplex(duplex) || + !(*validate)(cmd)) + return -EINVAL; + *dev_speed = speed; + *dev_duplex = duplex; + + return 0; +} +EXPORT_SYMBOL(ethtool_virtdev_set_link_ksettings); + /* Query device for its ethtool_cmd settings. * * Backward compatibility note: for compatibility with legacy ethtool, this is
Three virtual devices (ibmveth, virtio_net, and netvsc) all have similar code to 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@linux.vnet.ibm.com> --- include/linux/ethtool.h | 8 ++++++++ net/ethtool/ioctl.c | 44 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)