netdev - Re: [PATCH net-next 3/5] ethtool: add support to set/get rx buf len

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Aug 2021 08:09:08 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Guangbin Huang <huangguangbin2@...wei.com>
Cc:     <davem@...emloft.net>, <mkubecek@...e.cz>, <andrew@...n.ch>,
        <amitc@...lanox.com>, <idosch@...sch.org>, <danieller@...dia.com>,
        <netdev@...r.kernel.org>, <lipeng321@...wei.com>,
        <chenhao288@...ilicon.com>
Subject: Re: [PATCH net-next 3/5] ethtool: add support to set/get rx buf len

On Wed, 25 Aug 2021 14:40:53 +0800 Guangbin Huang wrote:
> From: Hao Chen <chenhao288@...ilicon.com>
> 
> Add support to set rx buf len via ethtool -G parameter and get
> rx buf len via ethtool -g parameter.
> 
> Signed-off-by: Hao Chen <chenhao288@...ilicon.com>
> Signed-off-by: Guangbin Huang <huangguangbin2@...wei.com>

> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 266e95e4fb33..6e26586274b3 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -516,6 +516,7 @@ struct ethtool_coalesce {
>   *	jumbo ring
>   * @tx_pending: Current maximum supported number of pending entries
>   *	per TX ring
> + * @rx_buf_len: Current supported size of rx ring BD buffer.

How about "Current length of buffers on the rx ring"?

>   * If the interface does not have separate RX mini and/or jumbo rings,
>   * @rx_mini_max_pending and/or @rx_jumbo_max_pending will be 0.
> @@ -533,6 +534,7 @@ struct ethtool_ringparam {
>  	__u32	rx_mini_pending;
>  	__u32	rx_jumbo_pending;
>  	__u32	tx_pending;
> +	__u32	rx_buf_len;
>  };

You can't extend this structure, because it's used by the IOCTL
interface directly. You need to pass the new value to the drivers 
in a different way. You can look at what Yufeng Mo did recently
for an example approach.

> @@ -105,6 +109,7 @@ const struct nla_policy ethnl_rings_set_policy[] = {
>  	[ETHTOOL_A_RINGS_RX_MINI]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_RINGS_RX_JUMBO]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_RINGS_TX]			= { .type = NLA_U32 },
> +	[ETHTOOL_A_RINGS_RX_BUF_LEN]            = { .type = NLA_U32 },

We should prevent user space for passing 0 as input, so we can use it
as a special value in the kernel. NLA_POLICY_MIN()

>  };
>  
>  int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
> @@ -142,6 +147,8 @@ int ethnl_set_rings(struct sk_buff *skb, struct genl_info *info)
>  	ethnl_update_u32(&ringparam.rx_jumbo_pending,
>  			 tb[ETHTOOL_A_RINGS_RX_JUMBO], &mod);
>  	ethnl_update_u32(&ringparam.tx_pending, tb[ETHTOOL_A_RINGS_TX], &mod);
> +	ethnl_update_u32(&ringparam.rx_buf_len,
> +			 tb[ETHTOOL_A_RINGS_RX_BUF_LEN], &mod);
>  	ret = 0;
>  	if (!mod)
>  		goto out_ops;

We need a way of preventing drivers which don't support the new option
from just silently ignoring it. Please add a capability like
cap_link_lanes_supported and reject non-0 ETHTOOL_A_RINGS_RX_BUF_LEN
if it's not set.