Re: [PATCH net-next v2 3/5] net: Revalidate queue config for ringparam changes

["Björn Töpel" <bjorn@kernel.org>]

netdev-bot+sashiko@kernel.org writes:

>>     Memory-provider queue configuration is validated when the provider is
>>     bound. A later ethtool ring change may invalidate it because drivers
>>     can size queue memory from both ring depth and RX page size. For
>>     example, fbnic uses multiple PPQ entries for each RX page larger than
>>     PAGE_SIZE, which reduces the usable software ring depth.
> [ ... ]
>>     Use the same transaction for ioctl and netlink. Drivers without
>>     ndo_validate_qcfg skip the new validation.
>
> [Severity: Low]
> The message reads as if it closes a live misconfiguration hole, but at this
> commit no in-tree driver implements a ring-depth-aware ndo_validate_qcfg and
> no driver reads qcfg->rx_ring_size, so netdev_queue_config_revalidate()
> cannot change any validation outcome yet.
>
> The two current ndo_validate_qcfg implementations, bnxt_validate_qcfg() and
> mlx5e_queue_validate_qcfg(), only inspect qcfg->rx_page_size. The fbnic
> consumer named in the message, fbnic_validate_qcfg() reading
> qcfg->rx_jumbo_ring_size, arrives in the following patch of the series.
>
> The last paragraph does disclose the limitation, so this is only about
> framing: could the message state that this is core plumbing and that the
> fbnic consumer lands in the next patch?

Ok! I reworded the commit message to state that fbnic consumer followes
the next patch.

...

>> diff --git a/net/core/netdev_config.c b/net/core/netdev_config.c
>> index b101341e32513..1975de42a60d9 100644
>> --- a/net/core/netdev_config.c
>> +++ b/net/core/netdev_config.c
>> @@ -50,6 +50,15 @@ static int netdev_nop_validate_qcfg(struct net_device *dev,
>>  	return 0;
>>  }
>>  
>> +static void netdev_qcfg_apply_dev(struct netdev_queue_config *qcfg,
>> +				  const struct netdev_config *cfg)
>> +{
>> +	/* Device config overrides callback-provided fallbacks. */
>> +	qcfg->rx_ring_size = cfg->rings.rx_pending;
>> +	qcfg->rx_mini_ring_size = cfg->rings.rx_mini_pending;
>> +	qcfg->rx_jumbo_ring_size = cfg->rings.rx_jumbo_pending;
>> +}
>> +
>
> [Severity: Low]
> Can these three assignments hand a rendered config full of zeroes to
> ndo_validate_qcfg()?

Yes, unless thte driver initializes the fields. Device-wide config is
authoritative -- including zero! I've clarified the requirements in the
docs for next version.

>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index 4b0bc503f9307..1320289025b25 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
> [ ... ]
>> @@ -2239,10 +2240,29 @@ static int ethtool_set_ringparam(struct net_device *dev, void __user *useraddr)
>>  	    ringparam.tx_pending > max.tx_max_pending)
>>  		return -EINVAL;
>>  
>> +	ret = netdev_reconfig_start(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +
>> +	ret = netdev_queue_config_revalidate(dev, NULL);
>> +	if (ret)
>> +		goto out_free_cfg;
>> +
>>  	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>>  					      &kernel_ringparam, NULL);
>> -	if (!ret)
>> -		ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
>> +	if (ret)
>> +		goto out_free_cfg;
>> +
>> +	/* The driver may adjust the accepted ring depths. */
>> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +	swap(dev->cfg, dev->cfg_pending);
>> +	ethtool_notify(dev, ETHTOOL_MSG_RINGS_NTF);
>
> [Severity: Low]
> Is the committed configuration guaranteed to be one that ndo_validate_qcfg()
> actually saw?

A driver implementing ndo_validate_qcfg must use the same normalization.
I documented that requirement, fbnic uses the same rounding helper in
both paths.

>> diff --git a/net/ethtool/rings.c b/net/ethtool/rings.c
>> index 9054c89c5d7bb..c04312fc0d061 100644
>> --- a/net/ethtool/rings.c
>> +++ b/net/ethtool/rings.c
> [ ... ]
>> @@ -299,10 +300,20 @@ ethnl_set_rings(struct ethnl_req_info *req_info, struct genl_info *info)
>>  
>>  	dev->cfg_pending->hds_config = kernel_ringparam.tcp_data_split;
>>  	dev->cfg_pending->hds_thresh = kernel_ringparam.hds_thresh;
>> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +
>> +	ret = netdev_queue_config_revalidate(dev, info->extack);
>> +	if (ret)
>> +		return ret;
>>  
>>  	ret = dev->ethtool_ops->set_ringparam(dev, &ringparam,
>>  					      &kernel_ringparam, info->extack);
>> -	return ret < 0 ? ret : 1;
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* The driver may adjust the accepted ring depths. */
>> +	ethtool_ringparam_set_cfg(dev->cfg_pending, &ringparam);
>> +	return 1;
>>  }
>
> [Severity: Low]
> The comment says the driver may adjust the accepted ring depths, but does
> this second copy store the depths the hardware ended up with, or just the
> requested ones?

It captures only adjustments that the driver reports through ringparam.
I updated the comments and docs.


Björn