netdev - Re: [PATCH net 2/2] i40e: fix xdp_redirect logs error message when testing with MTU=1500

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:   Tue, 15 Nov 2022 13:27:06 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <edumazet@...gle.com>,
        Bartosz Staszewski <bartoszx.staszewski@...el.com>,
        <netdev@...r.kernel.org>, <magnus.karlsson@...el.com>,
        <ast@...nel.org>, <daniel@...earbox.net>, <hawk@...nel.org>,
        <john.fastabend@...il.com>, <bpf@...r.kernel.org>,
        Mateusz Palczewski <mateusz.palczewski@...el.com>,
        Shwetha Nagaraju <Shwetha.nagaraju@...el.com>
Subject: Re: [PATCH net 2/2] i40e: fix xdp_redirect logs error message when
 testing with MTU=1500

On Mon, Nov 14, 2022 at 04:03:24PM -0800, Tony Nguyen wrote:
> From: Bartosz Staszewski <bartoszx.staszewski@...el.com>
> 
> The driver is currently logging an error message "MTU too large to enable XDP"
> when trying to enable XDP on totally normal MTU.

Could you rephrase this to "Fix the inability to attach XDP program on
downed interface" ?

> 
> This was caused by whenever the interface was down, function
> i40e_xdp was passing vsi->rx_buf_len field to i40e_xdp_setup()
> which was equal 0. i40e_open() then  calls i40e_vsi_configure_rx()
> which configures that field, but that only happens when interface is up.
> When it is down, i40e_open() is not being called, thus vsi->rx_buf_len
> which causes the bug is not set.

Where rx_buf_len is cleared though? Or is it only the case for a fresh
start?

> 
> Solution for this is calculate buffer length in newly created
> function - i40e_calculate_vsi_rx_buf_len() that return actual buffer
> length. Buffer length is being calculated based on the same rules applied
> previously in i40e_vsi_configure_rx() function.
> 
> Fixes: 613142b0bb88 ("i40e: Log error for oversized MTU on device")

I think the problem dates back to 2017 and:
Fixes: 0c8493d90b6b ("i40e: add XDP support for pass and drop actions")

CC: Bjorn Topel <bjorn@...nel.org>

So i'm saying let's have two fixes tags here.

> Signed-off-by: Bartosz Staszewski <bartoszx.staszewski@...el.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Tested-by: Shwetha Nagaraju <Shwetha.nagaraju@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 42 +++++++++++++++------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 41112f92f9ef..4b3b6e5b612d 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -3695,6 +3695,30 @@ static int i40e_vsi_configure_tx(struct i40e_vsi *vsi)
>  	return err;
>  }
>  
> +/**
> + * i40e_calculate_vsi_rx_buf_len - Calculates buffer length
> + *
> + * @vsi: VSI to calculate rx_buf_len from
> + */
> +static u16 i40e_calculate_vsi_rx_buf_len(struct i40e_vsi *vsi)
> +{
> +	u16 ret;
> +
> +	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> +		ret = I40E_RXBUFFER_2048;
> +#if (PAGE_SIZE < 8192)
> +	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> +		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +		ret = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> +#endif
> +	} else {
> +		ret = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> +					   I40E_RXBUFFER_2048;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * i40e_vsi_configure_rx - Configure the VSI for Rx
>   * @vsi: the VSI being configured
> @@ -3706,20 +3730,14 @@ static int i40e_vsi_configure_rx(struct i40e_vsi *vsi)
>  	int err = 0;
>  	u16 i;
>  
> -	if (!vsi->netdev || (vsi->back->flags & I40E_FLAG_LEGACY_RX)) {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = I40E_RXBUFFER_2048;
> +	vsi->max_frame = I40E_MAX_RXBUFFER;
> +	vsi->rx_buf_len = i40e_calculate_vsi_rx_buf_len(vsi);
> +
>  #if (PAGE_SIZE < 8192)
> -	} else if (!I40E_2K_TOO_SMALL_WITH_PADDING &&
> -		   (vsi->netdev->mtu <= ETH_DATA_LEN)) {
> +	if (vsi->netdev && !I40E_2K_TOO_SMALL_WITH_PADDING &&
> +	    vsi->netdev->mtu <= ETH_DATA_LEN)
>  		vsi->max_frame = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
> -		vsi->rx_buf_len = I40E_RXBUFFER_1536 - NET_IP_ALIGN;
>  #endif
> -	} else {
> -		vsi->max_frame = I40E_MAX_RXBUFFER;
> -		vsi->rx_buf_len = (PAGE_SIZE < 8192) ? I40E_RXBUFFER_3072 :
> -						       I40E_RXBUFFER_2048;
> -	}
>  
>  	/* set up individual rings */
>  	for (i = 0; i < vsi->num_queue_pairs && !err; i++)
> @@ -13267,7 +13285,7 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>  	int i;
>  
>  	/* Don't allow frames that span over multiple buffers */
> -	if (frame_size > vsi->rx_buf_len) {
> +	if (frame_size > i40e_calculate_vsi_rx_buf_len(vsi)) {
>  		NL_SET_ERR_MSG_MOD(extack, "MTU too large to enable XDP");
>  		return -EINVAL;
>  	}
> -- 
> 2.35.1
>