netdev - RE: [RFC PATCH V2] ethtool: Add command to configure IOV features

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] [day] [month] [year] [list]

Date:	Thu, 13 Oct 2011 10:07:27 -0700
From:	"Rose, Gregory V" <gregory.v.rose@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [RFC PATCH V2] ethtool:  Add command to configure IOV features

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Friday, October 07, 2011 5:41 PM
> To: Rose, Gregory V
> Cc: netdev@...r.kernel.org
> Subject: Re: [RFC PATCH V2] ethtool: Add command to configure IOV features
> 
> On Thu, 2011-09-22 at 14:35 -0700, Greg Rose wrote:
> > New command to allow configuration of IOV features such as the number of
> > Virtual Functions to allocate for a given Physical Function interface,
> > the number of semi-independent net devices to allocate from partitioned
> > I/O resources in the PF and to set the number of queues per VF.
> [...]
> > @@ -266,6 +270,8 @@ static struct option {
> >      { "-W", "--set-dump", MODE_SET_DUMP,
> >  		"Set dump flag of the device",
> >  		"		N\n"},
> > +    { "-v", "--get-iov", MODE_GET_IOV, "Get IOV parameters", "\n" },
> > +    { "-V", "--set_iov", MODE_SET_IOV, "Set IOV parameters", "[ N ]\n"
> },
> 
> Doesn't match the implementation.

Whoops... OK.

> 
> [...]
> > +static int do_get_iov(int fd, struct ifreq *ifr)
> > +{
> > +	int err;
> > +	struct ethtool_iov_get_cmd iov_cmd;
> > +
> > +	iov_cmd.cmd = ETHTOOL_IOV_GET_CMD;
> > +	ifr->ifr_data = (caddr_t)&iov_cmd;
> > +	err = send_ioctl(fd, ifr);
> > +	if (err < 0) {
> > +		perror("Can not get current IOV mode\n");
> > +		return 1;
> > +	}
> > +
> > +	memcpy(&iov_cmd, ifr->ifr_data, sizeof(iov_cmd));
> 
> But ifr->ifr_data == &iov_cmd.  So this is both pointless and dangerous
> (as memcpy() doesn't handle overlapping source and destination).

Huh... what was I thinking?  Yeah, I'll fix that.

> 
> [...]
> > +static int do_set_iov(int fd, struct ifreq *ifr)
> > +{
> > +	int err;
> > +	struct ethtool_iov_set_cmd iov_cmd;
> > +
> > +	if (iov_changed) {
> > +		iov_cmd.cmd = ETHTOOL_IOV_SET_CMD;
> > +		if (iov_numvfs_wanted >= 0) {
> > +			iov_cmd.set_cmd = ETHTOOL_IOV_CMD_CONFIGURE_SRIOV;
> > +			iov_cmd.cmd_param = iov_numvfs_wanted;
> > +		} else if (iov_numnetdevs_wanted >= 0) {
> > +			iov_cmd.set_cmd = ETHTOOL_IOV_CMD_CONFIGURE_NETDEVS;
> > +			iov_cmd.cmd_param = iov_numnetdevs_wanted;
> > +		} else if (iov_numvqueues_wanted >= 0) {
> > +			iov_cmd.set_cmd = ETHTOOL_IOV_CMD_CONFIGURE_VF_QUEUES;
> > +			iov_cmd.cmd_param = iov_numvqueues_wanted;
> > +		} else {
> > +			return -EINVAL;
> > +		}
> [...]
> 
> So what if the user specifies multiple keywords?

I hadn't contemplated that.

> 
> Also missing an update to the manual page.

When I get through the RFC process and we've settled on something acceptable to the community I'll update the man page.

Thanks for the review Ben.

- Greg