netdev - Re: [PATCH V3 3/6] net/ethtool: support set coalesce per queue

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]

Date:	Sun, 17 Jan 2016 02:13:13 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	kan.liang@...el.com, netdev@...r.kernel.org, davem@...emloft.net,
	bwh@...nel.org
Cc:	jesse.brandeburg@...el.com, andi@...stfloor.org,
	f.fainelli@...il.com, alexander.duyck@...il.com,
	jeffrey.t.kirsher@...el.com, shannon.nelson@...el.com,
	carolyn.wyborny@...el.com, donald.c.skidmore@...el.com,
	mitch.a.williams@...el.com, ogerlitz@...lanox.com,
	edumazet@...gle.com, jiri@...lanox.com, sfeldma@...il.com,
	gospo@...ulusnetworks.com, sasha.levin@...cle.com,
	dsahern@...il.com, tj@...nel.org, cascardo@...hat.com,
	corbet@....net
Subject: Re: [PATCH V3 3/6] net/ethtool: support set coalesce per queue

On Thu, 2016-01-14 at 09:08 -0500, kan.liang@...el.com wrote:
[...]
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1773,6 +1773,63 @@ static int ethtool_get_per_queue_coalesce(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int ethtool_set_per_queue_coalesce(struct net_device *dev,
> +					  void __user *useraddr,
> +					  struct ethtool_per_queue_op *per_queue_opt)
> +{
> +	int bit, i, ret = 0;
> +	int queue_num = bitmap_weight(per_queue_opt->queue_mask, MAX_NUM_QUEUE);
> +	struct ethtool_coalesce *backup = NULL, *tmp = NULL;
> +	bool rollback = true;
> +
> +	if (!dev->ethtool_ops->set_per_queue_coalesce)
> +		return -EOPNOTSUPP;
> +
> +	if (!dev->ethtool_ops->get_per_queue_coalesce)
> +		rollback = false;

I think that get_per_queue_coalesce should be a required operation.
Why would a driver not implement both?

Then there's no need to make the rollback code conditional.

> +	useraddr += sizeof(*per_queue_opt);
> +
> +	if (rollback)
> +		tmp = backup = kmalloc(queue_num * sizeof(*backup), GFP_KERNEL);

Use kmalloc_array() as it checks for overflow (shouldn't be possible
here, but it's best to check).

> +	for_each_set_bit(bit, per_queue_opt->queue_mask, MAX_NUM_QUEUE) {
> +		struct ethtool_coalesce coalesce;
> +
> +		if (rollback) {
> +			if (dev->ethtool_ops->get_per_queue_coalesce(dev, bit, tmp)) {
> +				ret = -EFAULT;

This should propagate the error code from get_per_queue_coalesce rather
than substituting EFAULT.

> +				goto roll_back;
> +			}
> +			tmp += sizeof(struct ethtool_coalesce);
> +		}
> +
> +		if (copy_from_user(&coalesce, useraddr, sizeof(coalesce))) {
> +			ret = -EFAULT;
> +			goto roll_back;
> +		}
> +
> +		ret = dev->ethtool_ops->set_per_queue_coalesce(dev, bit, &coalesce);
> +		if (ret != 0)
> +			goto roll_back;
> +
> +		useraddr += sizeof(coalesce);
> +	}
> +
> +roll_back:
> +	if (rollback) {
> +		if (ret != 0) {
> +			tmp = backup;
> +			for_each_set_bit(i, per_queue_opt->queue_mask, bit - 1) {

The upper bound is excluded so it should be bit, not bit - 1.

Ben.

> +				dev->ethtool_ops->set_per_queue_coalesce(dev, i, tmp);
> +				tmp += sizeof(struct ethtool_coalesce);
> +			}
> +		}
> +		kfree(backup);
> +	}
> +	return ret;
> +}
[...]

-- 
Ben Hutchings
Theory and practice are closer in theory than in practice.
                                - John Levine, moderator of comp.compilers
Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)