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: <CAMArcTUGTF2Qr9=W_mcrA9au2jhm0Ru6hC+Nt3V2tk=LODQs+A@mail.gmail.com>
Date: Sat, 2 Nov 2024 03:02:54 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Praveen Kaligineedi <pkaligineedi@...gle.com>, 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

On Fri, Nov 1, 2024 at 11:29 PM Mina Almasry <almasrymina@...gle.com> wrote:
>
> Hi Taehee, sorry for the late reply. I was out on vacation and needed
> to catch up on some stuff when I got back.

Hi Mina,
Thank you so much for your review :)

>
> 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.

There are several same code in the ethtool core.
It returns EOPNOTSUPP consistently.
In the v3 series, Brett reviewed it.
So, I changed it from EINVAL to EOPNOTSUPP it was reasonable to me.
So I prefer EOPNOTSUPP but I will follow your decision.
What do you think?

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

I removed extack from the v3. because ethtool doesn't use extack for
the same logic. It was reasonable to me.

>
> > +       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.

How about setting maximum hds_threshold to 0?
The default value of hds_threshold of course 0.
I think gve code does not need to change much, just adding like below
will be okay.

I think if drivers don't support setting hds_threshold explicitly, it
is actually the same as support only 0.
So, there is no problem I think.

I didn't analyze gve driver code, So I might think it too optimistically.

#define GVE_HDS_THRESHOLD_MAX 0
kernel_ering->hds_thresh = GVE_HDS_THRESHOLD_MAX;
kernel_ering->hds_thresh_max = GVE_HDS_THRESHOLD_MAX;
...
.supported_ring_params  = ETHTOOL_RING_USE_TCP_DATA_SPLIT |
ETHTOOL_RING_USE_HDS_THRS,

ethtool command may show like this.
ethtool -g enp7s0f3np3
Ring parameters for enp7s0f3np3:
Pre-set maximums:
...
Header data split thresh:       0
Current hardware settings:
...
TCP data split:         on
Header data split thresh:       0

If a driver can't set up hds_threshold, ethtool command may show like this.
ethtool -g enp7s0f3np3
Ring parameters for enp7s0f3np3:
Pre-set maximums:
TX push buff len:       n/a
Header data split thresh:       n/a
Current hardware settings:
...
TCP data split:         on
Header data split thresh:       n/a

Thanks a lot!
Taehee Yoo

>
> Other than that, looks fine to me.
>
>
> --
> Thanks,
> Mina

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ