netdev - Re: [PATCH net-next v4 3/8] net/funeth: probing and netdev ops

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: <CAOkoqZmSKHD97dbyVhKw3pPNJqKS2x8PXhsPJb9dRTjoGNn31g@mail.gmail.com>
Date:   Thu, 6 Jan 2022 00:47:29 -0800
From:   Dimitris Michailidis <d.michailidis@...gible.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v4 3/8] net/funeth: probing and netdev ops

On Wed, Jan 5, 2022 at 9:46 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 5 Jan 2022 07:52:21 -0800 Dimitris Michailidis wrote:
> > On Tue, Jan 4, 2022 at 6:07 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > > On Mon,  3 Jan 2022 22:46:52 -0800 Dimitris Michailidis wrote:
> > > > This is the first part of the Fungible ethernet driver. It deals with
> > > > device probing, net_device creation, and netdev ops.
> > >
> > > > +static int fun_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> > > > +{
> > > > +     struct bpf_prog *old_prog, *prog = xdp->prog;
> > > > +     struct funeth_priv *fp = netdev_priv(dev);
> > > > +     bool reconfig;
> > > > +     int rc, i;
> > > > +
> > > > +     /* XDP uses at most one buffer */
> > > > +     if (prog && dev->mtu > XDP_MAX_MTU) {
> > > > +             netdev_err(dev, "device MTU %u too large for XDP\n", dev->mtu);
> > > > +             NL_SET_ERR_MSG_MOD(xdp->extack,
> > > > +                                "Device MTU too large for XDP");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     reconfig = netif_running(dev) && (!!fp->xdp_prog ^ !!prog);
> > > > +     if (reconfig) {
> > > > +             rc = funeth_close(dev);
> > >
> > > Please rework runtime reconfig to not do the close and then open thing.
> > > This will prevent users from reconfiguring their NICs at runtime.
> > > You should allocate the resources first, then take the datapath down,
> > > reconfigure, swap and free the old resources.
> >
> > I imagine you have in mind something like nfp_net_ring_reconfig() but that
> > doesn't work as well here. We have the linux part of the data path (ring memory,
> > interrupts, etc) and the device part, handled by FW. I can't clone the device
> > portion for a quick swap during downtime. Since it involves messages to FW
> > updating the device portion is by far the bulk of the work and it needs to be
> > during the downtime. Doing Linux allocations before downtime offers little
> > improvement I think.
>
> It does - real machines running real workloads will often be under
> memory pressure. I've even seen XDP enable / disable fail just due
> to memory fragmentation, with plenty free memory when device rings
> are large.

I am in the process of changing this code.

> > There is ongoing work for FW to be able to modify live queues. When that
> > is available I expect this function will be able to move in and out of XDP with
> > no downtime.
>
> > > > +static void fun_destroy_netdev(struct net_device *netdev)
> > > > +{
> > > > +     if (likely(netdev)) {
> > >
> > > defensive programming?
> >
> > Looks that way but I'd rather have this function work with any input.
>
> There's way too much defensive programming in this driver. Unless there
> is a legit code path which can pass netdev == NULL you should remove
> the check.

OK