netdev - Re: [PATCH ethtool 2/3] ethtool: Regularise handling of offload flags

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]

Message-ID: <20110223030321.GA23620@rere.qmqm.pl>
Date:	Wed, 23 Feb 2011 04:03:21 +0100
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH ethtool 2/3] ethtool: Regularise handling of offload
	flags

On Mon, Feb 21, 2011 at 04:59:08PM +0000, Ben Hutchings wrote:
> Use the new ETHTOOL_{G,S}FEATURES operations where available, and
> use the new structure and netif feature flags in any case.
[...]
> --- a/ethtool.c
> +++ b/ethtool.c
[...]  
>  static int do_soffload(int fd, struct ifreq *ifr)
>  {
> +	struct {
> +		struct ethtool_gfeatures cmd;
> +		struct ethtool_get_features_block data[1];
> +	} get_features;
> +	struct {
> +		struct ethtool_sfeatures cmd;
> +		struct ethtool_set_features_block data[1];
> +	} set_features;
>  	struct ethtool_value eval;
>  	int err, changed = 0;
> +	u32 value;
> +	int i;
[-]
> +	get_features.cmd.cmd = ETHTOOL_GFEATURES;
> +	get_features.cmd.size = ARRAY_SIZE(get_features.data);
> +	ifr->ifr_data = (caddr_t)&get_features;
> +	err = ioctl(fd, SIOCETHTOOL, ifr);
> +	if (err == 0) {
> +		set_features.cmd.cmd = ETHTOOL_SFEATURES;
> +		set_features.cmd.size = ARRAY_SIZE(set_features.data);
> +		set_features.data[0] = off_features;
> +
> +		for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
> +			value = off_feature_def[i].value;
> +			if (!(off_features.valid & value))
> +				continue;
> +			if (!(get_features.data[0].available & value)) {
> +				/* None of these features can be changed */
> +				fprintf(stderr,
> +					"Cannot set device %s settings: "
> +					"Operation not supported\n",
> +					off_feature_def[i].long_name);
> +			} else if (off_features.requested & value) {
> +				/* Some of these features can be
> +				 * enabled; mask out any that cannot
> +				 */
> +				set_features.data[0].requested &=
> +					~(value &
> +					  ~get_features.data[0].available);
> +			}
>  		}
[-]
> +		ifr->ifr_data = (caddr_t)&set_features;
> +		err = ioctl(fd, SIOCETHTOOL, ifr);
> +		if (err < 0) {
> +			perror("Cannot set device offload settings");
> +			return 1;
>  		}
[-]
> +		changed = !!set_features.data[0].valid;
[-]
> +		if (err & ETHTOOL_F_WISH)
> +			fprintf(stderr,
> +				"Cannot set device offload settings: "
> +				"Some requested features depend on others "
> +				"that are not currently enabled\n");
> +
> +		/* ETHTOOL_F_UNSUPPORTED should never be set as we
> +		 * checked for unsupported flags above.  Treat any
> +		 * other warning flags as unknown.
> +		 */
> +		if (err & ~ETHTOOL_F_WISH)
> +			fprintf(stderr,
> +				"Cannot set device offload settings: "
> +				"warning flags %#x",
> +				err & ~ETHTOOL_F_WISH);
> +	} else if (errno != EOPNOTSUPP && errno != EPERM) {
> +		perror("Cannot get device offload settings");
> +		return 1;
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(off_feature_def); i++) {
> +			if (!off_feature_def[i].cmd)
> +				continue;
> +			if (off_features.valid & off_feature_def[i].value) {
> +				changed = 1;
> +				eval.cmd = off_feature_def[i].cmd + 1;
> +				eval.data = !!(off_features.requested &
> +					       off_feature_def[i].value);
> +				ifr->ifr_data = (caddr_t)&eval;
> +				err = send_ioctl(fd, ifr);
> +				if (err) {
> +					fprintf(stderr,
> +						"Cannot set device %s settings: %m\n",
> +						off_feature_def[i].long_name);
> +					return 1;
> +				}
> +			}
>  		}
[-]
> +		if (off_features.valid & flags_dup_features) {
> +			changed = 1;
> +			eval.cmd = ETHTOOL_GFLAGS;
> +			eval.data = 0;
> +			ifr->ifr_data = (caddr_t)&eval;
> +			err = ioctl(fd, SIOCETHTOOL, ifr);
> +			if (err) {
> +				perror("Cannot get device flag settings");
> +				return 91;
> +			}
[-]
> +			eval.cmd = ETHTOOL_SFLAGS;
> +			eval.data &= ~(off_features.valid & flags_dup_features);
> +			eval.data |= (off_features.requested &
> +				      flags_dup_features);
> +
> +			err = ioctl(fd, SIOCETHTOOL, ifr);
> +			if (err) {
> +				perror("Cannot set device flag settings");
> +				return 92;
> +			}
>  		}
>  	}

I noticed that you assumed that ETHTOOL_SFEATURES can change all features
changeable. This was not the case for drivers not yet converted to
ndo_fix_features() as there could be features changeable only by old ops.
I implemented the kernel part for fixing that in patches sent earlier today.
This introduces ETHTOOL_F_COMPAT bit that indicates when the compatibility
fallback was used (requested features are not fully saved by kernel in
that case).

Best Regards,
Michał Mirosław

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html