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

Date:   Sat, 22 Apr 2017 20:40:22 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Jakub Kicinski <kubakici@...pl>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     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 17-04-22 07:24 PM, Jakub Kicinski wrote:
> On Thu, 20 Apr 2017 18:50:21 -0700, Jeff Kirsher wrote:
>> +static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter)
>> +{
>> +	if (nr_cpu_ids > MAX_XDP_QUEUES)
>> +		return 0;
>> +
>> +	return adapter->xdp_prog ? nr_cpu_ids : 0;
>> +}
> 
> Nit: AFAICT ixgbe_xdp_setup() will guarantee xdp_prog is not set if
> there are too many CPU ids.

Sure being a bit paranoid I guess.

> 
>> @@ -6120,10 +6193,21 @@ static int ixgbe_setup_all_tx_resources(struct ixgbe_adapter *adapter)
>>  		e_err(probe, "Allocation for Tx Queue %u failed\n", i);
>>  		goto err_setup_tx;
>>  	}
>> +	for (j = 0; j < adapter->num_xdp_queues; j++) {
>> +		err = ixgbe_setup_tx_resources(adapter->xdp_ring[j]);
>> +		if (!err)
>> +			continue;
>> +
>> +		e_err(probe, "Allocation for Tx Queue %u failed\n", j);
>> +		goto err_setup_tx;
>> +	}
>> +
>>  
> 
> Nit: extra line here

OK well I guess we can fix this if we need a respin anyways.

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

Thanks,
John