Re: [RFC net v2 1/2] af_packet: prepare the virtio header before reserving an RX frame

[Willem de Bruijn]

Jingxuan Xie wrote:
> From: jingxuanxie <jingxuanxie@xxxxxxxxxxxx>
> 
> An RX virtio-header conversion can fail after packet_current_rx_frame()
> has reserved space in a TPACKET_V3 block and increased its packet count.
> Releasing the copy lock on that error path does not remove the incomplete
> record from the block.

Can you give a little more detail on the exact bug: what state does
not get reverted on error, and how problematic is this.

> Prepare the header in a local variable before reserving a frame, and copy
> it to the reserved frame only after conversion succeeds. Keep drop
> accounting, notification and copy_skb cleanup on conversion failure.
> This also keeps conversion out of the receive queue critical section.
> 
> Assisted-by: LLM
> Signed-off-by: Jingxuan Xie <jingxuanxie@xxxxxxxxxxxx>
> ---
>  net/packet/af_packet.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 823e0911c910..7b3e1e27f777 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2273,6 +2273,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>  	__u32 ts_status;
>  	unsigned int slot_id = 0;
>  	int vnet_hdr_sz = 0;
> +	struct virtio_net_hdr vnet_hdr;
>  
>  	/* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT.
>  	 * We may add members to them until current aligned size without forcing
> @@ -2375,6 +2376,14 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>  			vnet_hdr_sz = 0;
>  		}
>  	}
> +	if (vnet_hdr_sz &&
> +	    virtio_net_hdr_from_skb(skb, &vnet_hdr, vio_le(), true, 0)) {
> +		drop_reason = SKB_DROP_REASON_PACKET_SOCK_ERROR;
> +		atomic_inc(&po->tp_drops);
> +		sk->sk_data_ready(sk);
> +		sk_skb_reason_drop(sk, copy_skb, drop_reason);
> +		goto drop_n_restore;

Probably prefer a goto drop_n_account_unlocked rather than open coding
that here.

> +	}
>  	spin_lock(&sk->sk_receive_queue.lock);
>  	h.raw = packet_current_rx_frame(po, skb,
>  					TP_STATUS_KERNEL, (macoff+snaplen));
> @@ -2388,16 +2397,9 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>  		__set_bit(slot_id, po->rx_ring.rx_owner_map);
>  	}
>  
> -	if (vnet_hdr_sz &&
> -	    virtio_net_hdr_from_skb(skb, h.raw + macoff -
> -				    sizeof(struct virtio_net_hdr),
> -				    vio_le(), true, 0)) {
> -		if (po->tp_version <= TPACKET_V2)
> -			__clear_bit(slot_id, po->rx_ring.rx_owner_map);
> -		else
> -			prb_clear_blk_fill_status(&po->rx_ring);
> -		goto drop_n_account;
> -	}
> +	if (vnet_hdr_sz)
> +		memcpy(h.raw + macoff - sizeof(vnet_hdr), &vnet_hdr,
> +		       sizeof(vnet_hdr));
>  
>  	if (po->tp_version <= TPACKET_V2) {
>  		packet_increment_rx_head(po, &po->rx_ring);
> -- 
> 2.25.1