[PATCH v2 0/4] ethtool: correct {GS}CHANNELS and {GS}RXFH conflict

[PATCH v2 0/4] ethtool: correct {GS}CHANNELS and {GS}RXFH conflict

[Jacob Keller]

This patch series fixes up ethtool_set_channels operation which
allowed modifying the RXFH table indirectly by reducing the number of
queues below the current max queue used by the Rx flow table. Most
drivers incorrectly allowed this to destroy the Rx flow table and
would then start by reinitializing it to default settings. However,
drivers are not able to correctly handle the conflict since there was
no way to differentiate between the default settings and the user
requested explicit settings.

To fix this, implement a new netdev private flag which we use to
indicate whether the RXFH has been user configured. If someone has
a better alternative of how to store this information, let me know.
I am not sure that priv_flags is the best solution but I have not had
any better idea.

Secondly, we add a function which just calls the driver's get_rxfh
callback to determine the current indirection table. Loop through this
and we can determine the current highest queue that will be used by
RSS.

Now, modify ethtool_set_channels to add a check ensuring that if (a)
we have had rxfh configured by user, (b) we can get the maximum RSS
queue currently used, then we ensure that the newly requested Rx count
(or combined count) is at least as high as this maximum RSS queue. The
reasoning here is that we can always safely increase the number of
queues. If we decrease the queues we must ensure that the decrease
does not go lower than the highest in-use queue for the Rx flow table.

Drivers may still need to be patched if they currently overwrite the
Rx flow table during channel configuration. If the driver currently
always resets Rx flow table when increasing number of queues it must
be patched to only do this when netif_is_rxfh_configured returns
false.

The second patch simply adds a check to ensure that all provided
channel counts fit within driver defined maximums.

The third patch fixes fm10k to correctly reconfigure the RSS reta
table whenever it is still unconfigured. This means that the default
state will provide RSS to every queue. Once the user has configured
RXFH, then we should maintain it. In addition, since the case where we
must reconfigure the RSS table in this case should now no longer
occur, add a dev_err message to indicate the user that we did so.

I have also supplied an ethtool patch to enable setting the default Rx
flow indirection table. Without this, current ethtool does not support
sending an indir_size of 0, and thus does not correctly support
configuring back to the default.

Changes in v2:
* fixed compile error
* fixed incorrect comparison with max_rx_in_use
* adjusted looping over dev_size
* removed inline on function
* dropped patch about separating combined vs asymmetric channels
* verified behavior using fm10k driver

Jacob Keller (3):
  ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}
  ethtool: ensure channel counts are within bounds during SCHANNELS
  fm10k: don't reinitialize RSS flow table when RXFH configured

 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 10 +++-
 include/linux/netdevice.h                     |  8 +++
 net/core/ethtool.c                            | 71 ++++++++++++++++++++++++++-
 3 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.7.0.236.gda096a0.dirty

[PATCH 1/4] ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

[Jacob Keller]

Ethernet drivers implementing both {GS}RXFH and {GS}CHANNELS ethtool ops
incorrectly allow SCHANNELS when it would conflict with the settings
from SRXFH. This occurs because it is not possible for drivers to
understand whether their Rx flow indirection table has been configured
or is in the default state. In addition, drivers currently behave in
various ways when increasing the number of Rx channels.

Some drivers will always destroy the Rx flow indirection table when this
occurs, whether it has been set by the user or not. Other drivers will
attempt to preserve the table even if the user has never modified it
from the default driver settings. Neither of these situation is
desirable because it leads to unexpected behavior or loss of user
configuration.

The correct behavior is to simply return -EINVAL when SCHANNELS would
conflict with the current Rx flow table settings. However, it should
only do so if the current settings were modified by the user. If we
required that the new settings never conflict with the current (default)
Rx flow settings, we would force users to first reduce their Rx flow
settings and then reduce the number of Rx channels.

This patch proposes a solution implemented in net/core/ethtool.c which
ensures that all drivers behave correctly. It checks whether the RXFH
table has been configured to non-default settings, and stores this
information in a private netdev flag. When the number of channels is
requested to change, it first ensures that the current Rx flow table is
not going to assign flows to now disabled channels.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 include/linux/netdevice.h |  8 +++++++
 net/core/ethtool.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 289c2314d766..044fa2d88c7f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1291,6 +1291,7 @@ struct net_device_ops {
  * @IFF_OPENVSWITCH: device is a Open vSwitch master
  * @IFF_L3MDEV_SLAVE: device is enslaved to an L3 master device
  * @IFF_TEAM: device is a team device
+ * @IFF_RXFH_CONFIGURED: device has had Rx Flow indirection table configured
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1318,6 +1319,7 @@ enum netdev_priv_flags {
 	IFF_OPENVSWITCH			= 1<<22,
 	IFF_L3MDEV_SLAVE		= 1<<23,
 	IFF_TEAM			= 1<<24,
+	IFF_RXFH_CONFIGURED		= 1<<25,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1345,6 +1347,7 @@ enum netdev_priv_flags {
 #define IFF_OPENVSWITCH			IFF_OPENVSWITCH
 #define IFF_L3MDEV_SLAVE		IFF_L3MDEV_SLAVE
 #define IFF_TEAM			IFF_TEAM
+#define IFF_RXFH_CONFIGURED		IFF_RXFH_CONFIGURED
 
 /**
  *	struct net_device - The DEVICE structure.
@@ -4045,6 +4048,11 @@ static inline bool netif_is_lag_port(const struct net_device *dev)
 	return netif_is_bond_slave(dev) || netif_is_team_port(dev);
 }
 
+static inline bool netif_is_rxfh_configured(const struct net_device *dev)
+{
+	return dev->priv_flags & IFF_RXFH_CONFIGURED;
+}
+
 /* This device needs to keep skb dst for qdisc enqueue or ndo_start_xmit() */
 static inline void netif_keep_dst(struct net_device *dev)
 {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index daf04709dd3c..59aebaf9ed54 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -642,6 +642,37 @@ void netdev_rss_key_fill(void *buffer, size_t len)
 }
 EXPORT_SYMBOL(netdev_rss_key_fill);
 
+static int ethtool_get_max_rxfh_channel(struct net_device *dev, u32 *max)
+{
+	u32 dev_size, current_max = 0;
+	u32 *indir;
+	int ret;
+
+	if (!dev->ethtool_ops->get_rxfh_indir_size ||
+	    !dev->ethtool_ops->get_rxfh)
+		return -EOPNOTSUPP;
+	dev_size = dev->ethtool_ops->get_rxfh_indir_size(dev);
+	if (dev_size == 0)
+		return -EOPNOTSUPP;
+
+	indir = kcalloc(dev_size, sizeof(indir[0]), GFP_USER);
+	if (!indir)
+		return -ENOMEM;
+
+	ret = dev->ethtool_ops->get_rxfh(dev, indir, NULL, NULL);
+	if (ret)
+		goto out;
+
+	while (dev_size--)
+		current_max = max(current_max, indir[dev_size]);
+
+	*max = current_max;
+
+out:
+	kfree(indir);
+	return ret;
+}
+
 static noinline_for_stack int ethtool_get_rxfh_indir(struct net_device *dev,
 						     void __user *useraddr)
 {
@@ -738,6 +769,14 @@ static noinline_for_stack int ethtool_set_rxfh_indir(struct net_device *dev,
 	}
 
 	ret = ops->set_rxfh(dev, indir, NULL, ETH_RSS_HASH_NO_CHANGE);
+	if (ret)
+		goto out;
+
+	/* indicate whether rxfh was set to default */
+	if (user_size == 0)
+		dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
+	else
+		dev->priv_flags |= IFF_RXFH_CONFIGURED;
 
 out:
 	kfree(indir);
@@ -897,6 +936,14 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	}
 
 	ret = ops->set_rxfh(dev, indir, hkey, rxfh.hfunc);
+	if (ret)
+		goto out;
+
+	/* indicate whether rxfh was set to default */
+	if (rxfh.indir_size == 0)
+		dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
+	else if (rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE)
+		dev->priv_flags |= IFF_RXFH_CONFIGURED;
 
 out:
 	kfree(rss_config);
@@ -1228,6 +1275,7 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
 	struct ethtool_channels channels;
+	u32 max_rx_in_use = 0;
 
 	if (!dev->ethtool_ops->set_channels)
 		return -EOPNOTSUPP;
@@ -1235,6 +1283,13 @@ static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
+	/* ensure the new Rx count fits within the configured Rx flow
+	 * indirection table settings */
+	if (netif_is_rxfh_configured(dev) &&
+	    !ethtool_get_max_rxfh_channel(dev, &max_rx_in_use) &&
+	    (channels.combined_count + channels.rx_count) <= max_rx_in_use)
+	    return -EINVAL;
+
 	return dev->ethtool_ops->set_channels(dev, &channels);
 }
 
-- 
2.7.0.236.gda096a0.dirty

[PATCH 2/4] ethtool: ensure channel counts are within bounds during SCHANNELS

[Jacob Keller]

Add a sanity check to ensure that all requested channel sizes are within
bounds, which should reduce errors in driver implementation.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 net/core/ethtool.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 59aebaf9ed54..dc4f6632f33b 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1274,15 +1274,24 @@ static noinline_for_stack int ethtool_get_channels(struct net_device *dev,
 static noinline_for_stack int ethtool_set_channels(struct net_device *dev,
 						   void __user *useraddr)
 {
-	struct ethtool_channels channels;
+	struct ethtool_channels channels, max;
 	u32 max_rx_in_use = 0;
 
-	if (!dev->ethtool_ops->set_channels)
+	if (!dev->ethtool_ops->set_channels || !dev->ethtool_ops->get_channels)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&channels, useraddr, sizeof(channels)))
 		return -EFAULT;
 
+	dev->ethtool_ops->get_channels(dev, &max);
+
+	/* ensure new counts are within the maximums */
+	if ((channels.rx_count > max.max_rx) ||
+	    (channels.tx_count > max.max_tx) ||
+	    (channels.combined_count > max.max_combined) ||
+	    (channels.other_count > max.max_other))
+		return -EINVAL;
+
 	/* ensure the new Rx count fits within the configured Rx flow
 	 * indirection table settings */
 	if (netif_is_rxfh_configured(dev) &&
-- 
2.7.0.236.gda096a0.dirty

[PATCH 3/4] fm10k: don't reinitialize RSS flow table when RXFH configured

[Jacob Keller]

Also print an error message incase we do have to reconfigure as this
should no longer happen anymore due to ethtool changes. If it somehow
does occur, user should be made aware of it.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 134ce4daa994..4e58f9155883 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1937,8 +1937,10 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
 	u16 i, rss_i = interface->ring_feature[RING_F_RSS].indices;
 	u32 reta, base;
 
-	/* If the netdev is initialized we have to maintain table if possible */
-	if (interface->netdev->reg_state != NETREG_UNINITIALIZED) {
+	/* If the Rx flow indirection table has been configured manually, we
+	 * need to maintain it when possible.
+	 */
+	if (netif_is_rxfh_configured(interface->netdev)) {
 		for (i = FM10K_RETA_SIZE; i--;) {
 			reta = interface->reta[i];
 			if ((((reta << 24) >> 24) < rss_i) &&
@@ -1946,6 +1948,10 @@ static void fm10k_init_reta(struct fm10k_intfc *interface)
 			    (((reta <<  8) >> 24) < rss_i) &&
 			    (((reta)       >> 24) < rss_i))
 				continue;
+
+			/* this should never happen */
+			dev_err(&interface->pdev->dev,
+				"RSS indirection table assigned flows out of queue bounds. Reconfiguring.\n");
 			goto repopulate_reta;
 		}
 
-- 
2.7.0.236.gda096a0.dirty

[PATCH v2 4/4] ethtool: support setting default Rx flow indirection table

[Jacob Keller]

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 ethtool.8.in |  5 ++++-
 ethtool.c    | 46 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/ethtool.8.in b/ethtool.8.in
index eeffa70415b5..b7d56fbe4484 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -296,7 +296,7 @@ ethtool \- query or control network driver and hardware settings
 .IR N \ |
 .BI weight\  W0
 .IR W1
-.RB ...\ ]
+.RB ...\ | \ default \ ]
 .HP
 .B ethtool \-f|\-\-flash
 .I devname file
@@ -805,6 +805,9 @@ Sets the receive flow hash indirection table to spread flows between
 receive queues according to the given weights.  The sum of the weights
 must be non-zero and must not exceed the size of the indirection table.
 .TP
+.BI default
+Sets the receive flow hash indirection table to its default value.
+.TP
 .B \-f \-\-flash
 Write a firmware image to flash or other non-volatile memory on the
 device.
diff --git a/ethtool.c b/ethtool.c
index 92c40b823f2c..c05b50f830a7 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3228,13 +3228,11 @@ static int do_grxfh(struct cmd_context *ctx)
 	return 0;
 }
 
-static int fill_indir_table(u32 *indir_size, u32 *indir, int rxfhindir_equal,
-			    char **rxfhindir_weight, u32 num_weights)
+static int fill_indir_table(u32 *indir_size, u32 *indir, int rxfhindir_default,
+			    int rxfhindir_equal, char **rxfhindir_weight,
+			    u32 num_weights)
 {
 	u32 i;
-	/*
-	 * "*indir_size == 0" ==> reset indir to default
-	 */
 	if (rxfhindir_equal) {
 		for (i = 0; i < *indir_size; i++)
 			indir[i] = i % rxfhindir_equal;
@@ -3268,6 +3266,9 @@ static int fill_indir_table(u32 *indir_size, u32 *indir, int rxfhindir_equal,
 			}
 			indir[i] = j;
 		}
+	} else if (rxfhindir_default) {
+		/* "*indir_size == 0" ==> reset indir to default */
+		*indir_size = 0;
 	} else {
 		*indir_size = ETH_RXFH_INDIR_NO_CHANGE;
 	}
@@ -3275,8 +3276,9 @@ static int fill_indir_table(u32 *indir_size, u32 *indir, int rxfhindir_equal,
 	return 0;
 }
 
-static int do_srxfhindir(struct cmd_context *ctx, int rxfhindir_equal,
-			 char **rxfhindir_weight, u32 num_weights)
+static int do_srxfhindir(struct cmd_context *ctx, int rxfhindir_default,
+			 int rxfhindir_equal, char **rxfhindir_weight,
+			 u32 num_weights)
 {
 	struct ethtool_rxfh_indir indir_head;
 	struct ethtool_rxfh_indir *indir;
@@ -3301,7 +3303,8 @@ static int do_srxfhindir(struct cmd_context *ctx, int rxfhindir_equal,
 	indir->cmd = ETHTOOL_SRXFHINDIR;
 	indir->size = indir_head.size;
 
-	if (fill_indir_table(&indir->size, indir->ring_index, rxfhindir_equal,
+	if (fill_indir_table(&indir->size, indir->ring_index,
+			     rxfhindir_default, rxfhindir_equal,
 			     rxfhindir_weight, num_weights)) {
 		free(indir);
 		return 1;
@@ -3323,7 +3326,7 @@ static int do_srxfh(struct cmd_context *ctx)
 	struct ethtool_rxfh rss_head = {0};
 	struct ethtool_rxfh *rss;
 	struct ethtool_rxnfc ring_count;
-	int rxfhindir_equal = 0;
+	int rxfhindir_equal = 0, rxfhindir_default = 0;
 	char **rxfhindir_weight = NULL;
 	char *rxfhindir_key = NULL;
 	char *hkey = NULL;
@@ -3332,7 +3335,7 @@ static int do_srxfh(struct cmd_context *ctx)
 	u32 entry_size = sizeof(rss_head.rss_config[0]);
 	u32 num_weights = 0;
 
-	if (ctx->argc < 2)
+	if (ctx->argc < 1)
 		exit_bad_args();
 
 	while (arg_num < ctx->argc) {
@@ -3357,6 +3360,9 @@ static int do_srxfh(struct cmd_context *ctx)
 			if (!rxfhindir_key)
 				exit_bad_args();
 			++arg_num;
+		} else if (!strcmp(ctx->argp[arg_num], "default")) {
+			++arg_num;
+			rxfhindir_default = 1;
 		} else {
 			exit_bad_args();
 		}
@@ -3368,6 +3374,18 @@ static int do_srxfh(struct cmd_context *ctx)
 		return 1;
 	}
 
+	if (rxfhindir_equal && rxfhindir_default) {
+		fprintf(stderr,
+			"Equal and default options are mutually exclusive\n");
+		return 1;
+	}
+
+	if (rxfhindir_weight && rxfhindir_default) {
+		fprintf(stderr,
+			"Weight and default options are mutually exclusive\n");
+		return 1;
+	}
+
 	ring_count.cmd = ETHTOOL_GRXRINGS;
 	err = send_ioctl(ctx, &ring_count);
 	if (err < 0) {
@@ -3378,8 +3396,8 @@ static int do_srxfh(struct cmd_context *ctx)
 	rss_head.cmd = ETHTOOL_GRSSH;
 	err = send_ioctl(ctx, &rss_head);
 	if (err < 0 && errno == EOPNOTSUPP && !rxfhindir_key) {
-		return do_srxfhindir(ctx, rxfhindir_equal, rxfhindir_weight,
-				     num_weights);
+		return do_srxfhindir(ctx, rxfhindir_default, rxfhindir_equal,
+				     rxfhindir_weight, num_weights);
 	} else if (err < 0) {
 		perror("Cannot get RX flow hash indir size and key size");
 		return 1;
@@ -3404,8 +3422,8 @@ static int do_srxfh(struct cmd_context *ctx)
 	rss->indir_size = rss_head.indir_size;
 	rss->key_size = rss_head.key_size;
 
-	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_equal,
-			     rxfhindir_weight, num_weights)) {
+	if (fill_indir_table(&rss->indir_size, rss->rss_config, rxfhindir_default,
+			     rxfhindir_equal, rxfhindir_weight, num_weights)) {
 		err = 1;
 		goto free;
 	}
-- 
2.7.0.236.gda096a0.dirty

Re: [PATCH v2 0/4] ethtool: correct {GS}CHANNELS and {GS}RXFH conflict

[David Miller]

From: Jacob Keller <jacob.e.keller@intel.com>
Date: Mon,  8 Feb 2016 16:05:02 -0800

> This patch series fixes up ethtool_set_channels operation which
> allowed modifying the RXFH table indirectly by reducing the number of
> queues below the current max queue used by the Rx flow table. Most
> drivers incorrectly allowed this to destroy the Rx flow table and
> would then start by reinitializing it to default settings. However,
> drivers are not able to correctly handle the conflict since there was
> no way to differentiate between the default settings and the user
> requested explicit settings.
 ...

Patches 1-3 applied to net-next, thanks.

Re: [PATCH v2 4/4] ethtool: support setting default Rx flow indirection table

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On Tue, 2016-02-16 at 21:22 +0000, Keller, Jacob E wrote:

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> 
> Not sure if there is a mailing list for this, I sent this to the netdev
> list but forgot to Cc you on the ethtool change.

I haven't been keeping up with netdev for a long time, but I have
recently set up filtering by subject so I can keep up with just the
ethtool-related messages.  Still, patches for the ethtool command
should always be explicitly sent to me.

> Dave applied the
> network core patches, but they're more or less useless unless we
> actually have the ability to request default setting using ethtool
> (which I extended to support "default" here)

The patch was mangled (word-wrapped and modified white-space) in this
message, so I took the version in
<http://article.gmane.org/gmane.linux.network/398404/>.

[...]
> @@ -3332,7 +3335,7 @@ static int do_srxfh(struct cmd_context *ctx)
>  	u32 entry_size = sizeof(rss_head.rss_config[0]);
>  	u32 num_weights = 0;
>  
> -	if (ctx->argc < 2)
> +	if (ctx->argc < 1)
>  		exit_bad_args();
[...]

This means we might continue without having the required parameter
after "equal", "weight" or "hkey".  But, having said that, since we're
only checking once before running the loop, we're already failing to
validate that properly.

I've applied this, but could you please send another patch that adds
checks on ctx->argc within the loop and test cases in test-cmdline.c?

Ben.

-- 
Ben Hutchings
If at first you don't succeed, you're doing about average.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH v2 4/4] ethtool: support setting default Rx flow indirection table

[Keller, Jacob E]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Sunday, March 13, 2016 9:25 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: netdev <netdev@vger.kernel.org>
> Subject: Re: [PATCH v2 4/4] ethtool: support setting default Rx flow
> indirection table
> 
> On Tue, 2016-02-16 at 21:22 +0000, Keller, Jacob E wrote:
> 
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >
> > Not sure if there is a mailing list for this, I sent this to the netdev
> > list but forgot to Cc you on the ethtool change.
> 
> I haven't been keeping up with netdev for a long time, but I have
> recently set up filtering by subject so I can keep up with just the
> ethtool-related messages.  Still, patches for the ethtool command
> should always be explicitly sent to me.
> 
> > Dave applied the
> > network core patches, but they're more or less useless unless we
> > actually have the ability to request default setting using ethtool
> > (which I extended to support "default" here)
> 
> The patch was mangled (word-wrapped and modified white-space) in this
> message, so I took the version in
> <http://article.gmane.org/gmane.linux.network/398404/>.
> 
> [...]
> > @@ -3332,7 +3335,7 @@ static int do_srxfh(struct cmd_context *ctx)
> >  	u32 entry_size = sizeof(rss_head.rss_config[0]);
> >  	u32 num_weights = 0;
> >
> > -	if (ctx->argc < 2)
> > +	if (ctx->argc < 1)
> >  		exit_bad_args();
> [...]
> 
> This means we might continue without having the required parameter
> after "equal", "weight" or "hkey".  But, having said that, since we're
> only checking once before running the loop, we're already failing to
> validate that properly.
> 
> I've applied this, but could you please send another patch that adds
> checks on ctx->argc within the loop and test cases in test-cmdline.c?
> 
> Ben.
> 

Yes. Not sure how the patch got broken for you here, as I sent it using git-send-email. I will send the proposed fix above.

Thanks,
Jake