netdev - Re: [PATCH net-next v4 5/8] net: devmem: add ring parameter filtering

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]

Message-ID: <CAHS8izNbS4i+ke0bK07-rNLuq6RGXD-H73DhVb1-tsUOzSCBog@mail.gmail.com>
Date: Fri, 1 Nov 2024 07:29:04 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Taehee Yoo <ap420073@...il.com>, Praveen Kaligineedi <pkaligineedi@...gle.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	edumazet@...gle.com, donald.hunter@...il.com, corbet@....net, 
	michael.chan@...adcom.com, andrew+netdev@...n.ch, hawk@...nel.org, 
	ilias.apalodimas@...aro.org, ast@...nel.org, daniel@...earbox.net, 
	john.fastabend@...il.com, dw@...idwei.uk, sdf@...ichev.me, 
	asml.silence@...il.com, brett.creeley@....com, linux-doc@...r.kernel.org, 
	netdev@...r.kernel.org, kory.maincent@...tlin.com, 
	maxime.chevallier@...tlin.com, danieller@...dia.com, hengqi@...ux.alibaba.com, 
	ecree.xilinx@...il.com, przemyslaw.kitszel@...el.com, hkallweit1@...il.com, 
	ahmed.zaki@...el.com, rrameshbabu@...dia.com, idosch@...dia.com, 
	jiri@...nulli.us, bigeasy@...utronix.de, lorenzo@...nel.org, 
	jdamato@...tly.com, aleksander.lobakin@...el.com, kaiyuanz@...gle.com, 
	willemb@...gle.com, daniel.zahka@...il.com
Subject: Re: [PATCH net-next v4 5/8] net: devmem: add ring parameter filtering

Hi Taehee, sorry for the late reply. I was out on vacation and needed
to catch up on some stuff when I got back.

On Tue, Oct 22, 2024 at 9:25 AM Taehee Yoo <ap420073@...il.com> wrote:
>
> If driver doesn't support ring parameter or tcp-data-split configuration
> is not sufficient, the devmem should not be set up.
> Before setup the devmem, tcp-data-split should be ON and
> header-data-split-thresh value should be 0.
>
> Tested-by: Stanislav Fomichev <sdf@...ichev.me>
> Signed-off-by: Taehee Yoo <ap420073@...il.com>
> ---
>
> v4:
>  - Check condition before __netif_get_rx_queue().
>  - Separate condition check.
>  - Add Test tag from Stanislav.
>
> v3:
>  - Patch added.
>
>  net/core/devmem.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 11b91c12ee11..3425e872e87a 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -8,6 +8,8 @@
>   */
>
>  #include <linux/dma-buf.h>
> +#include <linux/ethtool.h>
> +#include <linux/ethtool_netlink.h>
>  #include <linux/genalloc.h>
>  #include <linux/mm.h>
>  #include <linux/netdevice.h>
> @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>                                     struct net_devmem_dmabuf_binding *binding,
>                                     struct netlink_ext_ack *extack)
>  {
> +       struct kernel_ethtool_ringparam kernel_ringparam = {};
> +       struct ethtool_ringparam ringparam = {};
>         struct netdev_rx_queue *rxq;
>         u32 xa_idx;
>         int err;
> @@ -140,6 +144,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>                 return -ERANGE;
>         }
>
> +       if (!dev->ethtool_ops->get_ringparam)
> +               return -EOPNOTSUPP;
> +

Maybe an error code not EOPNOTSUPP. I think that gets returned when
NET_DEVMEM is not compiled in and other situations like that. Lets
pick another error code? Maybe ENODEV.

Also consider extack error message. But it's very unlikely to hit this
error, so maybe not necessary.

> +       dev->ethtool_ops->get_ringparam(dev, &ringparam, &kernel_ringparam,
> +                                       extack);
> +       if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
> +               NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
> +               return -EINVAL;
> +       }
> +       if (kernel_ringparam.hds_thresh) {
> +               NL_SET_ERR_MSG(extack, "header-data-split-thresh is not zero");
> +               return -EINVAL;
> +       }
> +

Thinking about drivers that support tcp-data-split, but don't support
any kind of hds_thresh. For us (GVE), the hds_thresh is implicitly
always 0.

Does the driver need to explicitly set hds_thresh to 0? If so, that
adds a bit of churn to driver code. Is it possible to detect in this
function that the driver doesn't support hds_thresh and allow the
binding if so?

I see in the previous patch you do something like:

supported_ring_params & ETHTOOL_RING_USE_HDS_THRS

To detect there is hds_thresh support. I was wondering if something
like this is possible so we don't have to update GVE and all future
drivers to explicitly set thresh to 0.

Other than that, looks fine to me.


-- 
Thanks,
Mina