netdev - Re: [PATCH, net-next, v3, 1/2] 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.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191220070418.GE21614@unicorn.suse.cz>
Date:   Fri, 20 Dec 2019 08:04:18 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Thomas Falcon <tlfalcon@...ux.ibm.com>,
        Cris Forno <cforno12@...ux.vnet.ibm.com>, mst@...hat.com,
        jasowang@...hat.com, haiyangz@...rosoft.com,
        sthemmin@...rosoft.com, sashal@...nel.org,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH, net-next, v3, 1/2] 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.

On Thu, Dec 19, 2019 at 07:26:14PM -0600, Thomas Falcon wrote:
> On 12/19/19 4:36 PM, Michal Kubecek wrote:
> > On Thu, Dec 19, 2019 at 01:40:56PM -0600, Cris Forno wrote:
[...]
> > > @@ -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)
> > > +{
> > > +	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) &&
> > Isn't this condition always true? You zeroed the advertising bitmap
> > above. Could you just omit this part and clearing of advertising above?
> > 
> > > +		bitmap_empty(diff1.link_modes.lp_advertising,
> > > +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
> > > +}
> > Another idea: instead of zeroing parts of diff1, you could copy these
> > members from *cmd to diff2 and compare cmd->base with diff2.base. You
> > could then drop diff1. And you wouldn't even need whole struct
> > ethtool_link_ksettings for diff2 as you only compare embedded struct
> > ethtool_link_settings (and check two bitmaps in cmd->link_modes).
> 
> If I understand your suggestion correctly, then the validate function might
> look something like this?
> 
> /* Check if the user is trying to change anything besides speed/duplex */
> static 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);
> }

Yes, that is what I wanted to suggest (the second argument of memcmp()
should be "&cmd->base", I think).

Michal