netdev - Re: [net-next 03/11] ixgbe: add support for XDP

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] [day] [month] [year] [list]

Message-ID: <20170422205226.1389d2dc@cakuba.lan>
Date:   Sat, 22 Apr 2017 20:52:25 -0700
From:   Jakub Kicinski <kubakici@...pl>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        jogreene@...hat.com, John Fastabend <john.r.fastabend@...el.com>
Subject: Re: [net-next 03/11] ixgbe: add support for XDP_TX action

On Sat, 22 Apr 2017 20:40:22 -0700, John Fastabend wrote:
> >> @@ -9557,7 +9739,21 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> >>  			return -EINVAL;
> >>  	}
> >>  
> >> +	if (nr_cpu_ids > MAX_XDP_QUEUES)
> >> +		return -ENOMEM;
> >> +
> >>  	old_prog = xchg(&adapter->xdp_prog, prog);
> >> +
> >> +	/* If transitioning XDP modes reconfigure rings */
> >> +	if (!!prog != !!old_prog) {
> >> +		int err = ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
> >> +
> >> +		if (err) {
> >> +			rcu_assign_pointer(adapter->xdp_prog, old_prog);
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +
> >>  	for (i = 0; i < adapter->num_rx_queues; i++)
> >>  		xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
> >>    
> > 
> > In case of disabling XDP I assume ixgbe_setup_tc() will free the rings
> > before the xdp_prog on the rings is swapped to NULL.  Is there anything
> > preventing TX in that time window?  I think usual ordering would be to
> > install the prog after reconfig but uninstall before.
> >   
> 
> Well in the ixgbe_setup_tc() case we set the rx_ring->xdp_prog in
> ixgbe_setup_rx_resorources(), while the dma engine is disabled, so the for
> loop is just doing another set on the rx_ring assigning it to the program
> already set previously.
> 
> Its not really buggy its just extra useless work so I'll change it to this,
> 
> 	if (!!prog != !!old_prog) {
> 		...
> 	} else {
> 		for ( ... )
> 			swap xdp prog
> 	}
> 
> Nice spot, thanks for reviewing. And I missed a build error so I'll roll these
> fixes in a resend.

Ah, thanks for explaining.  No bugs that I can spot then :)