[net-next,V1,1/4] net/mlx5e: Restore the skb data pointer after xmit is finished

From: Eran Ben Elisha <eranbe@mellanox.com>

Restore the skb data pointer after coping the data to the HW, so the skb
can be cloned with correct headers for future use (e.g timestamping).

Fixes: e586b3b0baee ("net/mlx5: Ethernet Datapath files")
Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Thu, 17 Dec 2015 14:35:32 +0200

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> index 1341b1d..0fcfe64 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
> @@ -165,6 +165,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>  	struct mlx5_wqe_eth_seg  *eseg = &wqe->eth;
>  	struct mlx5_wqe_data_seg *dseg;
>  
> +	unsigned char *skb_data_orig = skb->data;
>  	u8  opcode = MLX5_OPCODE_SEND;
>  	dma_addr_t dma_addr = 0;
>  	bool bf = false;
> @@ -263,6 +264,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>  	cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | opcode);
>  	cseg->qpn_ds           = cpu_to_be32((sq->sqn << 8) | ds_cnt);
>  
> +	skb_push(skb, skb->data - skb_data_orig);
>  	sq->skb[pi] = skb;
>  
>  	MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,

And in the middle of this we have:

		skb_pull_inline(skb, ihs);

This is looks illegal.

You must not modify the data pointers of any SKB that you receive for
sending via ->ndo_start_xmit() unless you know that absolutely you are
the one and only reference that exists to that SKB.

And exactly for the case you are trying to "fix" here, you do not.  If
the SKB is cloned, or has an elevated users count, someone else can be
looking at it exactly at the same time you are messing with the data
pointers.

I bet mlx4 has this bug too.

You must fix this properly, by keeping track of an offset or similar
internally to your driver, rather than changing the SKB data pointers.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 17, 2015 at 10:21 PM, David Miller <davem@davemloft.net> wrote:
> From: Saeed Mahameed <saeedm@mellanox.com>
> Date: Thu, 17 Dec 2015 14:35:32 +0200
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
>> index 1341b1d..0fcfe64 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
>> @@ -165,6 +165,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>>       struct mlx5_wqe_eth_seg  *eseg = &wqe->eth;
>>       struct mlx5_wqe_data_seg *dseg;
>>
>> +     unsigned char *skb_data_orig = skb->data;
>>       u8  opcode = MLX5_OPCODE_SEND;
>>       dma_addr_t dma_addr = 0;
>>       bool bf = false;
>> @@ -263,6 +264,7 @@ static netdev_tx_t mlx5e_sq_xmit(struct mlx5e_sq *sq, struct sk_buff *skb)
>>       cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | opcode);
>>       cseg->qpn_ds           = cpu_to_be32((sq->sqn << 8) | ds_cnt);
>>
>> +     skb_push(skb, skb->data - skb_data_orig);
>>       sq->skb[pi] = skb;
>>
>>       MLX5E_TX_SKB_CB(skb)->num_wqebbs = DIV_ROUND_UP(ds_cnt,
>
> And in the middle of this we have:
>
>                 skb_pull_inline(skb, ihs);
>
> This is looks illegal.
>
> You must not modify the data pointers of any SKB that you receive for
> sending via ->ndo_start_xmit() unless you know that absolutely you are
> the one and only reference that exists to that SKB.
>
> And exactly for the case you are trying to "fix" here, you do not.  If
> the SKB is cloned, or has an elevated users count, someone else can be
> looking at it exactly at the same time you are messing with the data
> pointers.

Agree, we will provide a fix soon.

>
> I bet mlx4 has this bug too.

I did a quick review and I din't see that we mess with SKB data pointer in mlx4.
if you know of such bug in mlx4, please share with us, we will handle ASAP.

> You must fix this properly, by keeping track of an offset or similar
> internally to your driver, rather than changing the SKB data pointers.
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html