[dpdk-dev] pcap: fix captured frame length

  The actual captured length is header.caplen, whereas header.len is
the original length on the wire.

Signed-off-by: Dror Birkman <dror.birkman@lightcyber.com>
---


Without this fix, if the captured length is smaller than the original
length on the wire, mbuf will contain incorrect data.


 drivers/net/pcap/rte_eth_pcap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
  

Hi Dror,

Good catch. What you are saying makes sense and it is also explained in 
pcap's documentation. Was your setup unusual though?
This might sound like a silly question but I don't remember seeing that 
issue and I should have since your fix is correct.

Nico.

On 28/01/16 11:09, Dror Birkman wrote:
> The actual captured length is header.caplen, whereas header.len is
> the original length on the wire.
>
> Signed-off-by: Dror Birkman <dror.birkman@lightcyber.com>
> ---
>
>
> Without this fix, if the captured length is smaller than the original
> length on the wire, mbuf will contain incorrect data.
>
>
>   drivers/net/pcap/rte_eth_pcap.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index f9230eb..1d121f8 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -220,25 +220,25 @@ eth_pcap_rx(void *queue,
>   		buf_size = (uint16_t)(rte_pktmbuf_data_room_size(pcap_q->mb_pool) -
>   				RTE_PKTMBUF_HEADROOM);
>   
> -		if (header.len <= buf_size) {
> +		if (header.caplen <= buf_size) {
>   			/* pcap packet will fit in the mbuf, go ahead and copy */
>   			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
> -					header.len);
> -			mbuf->data_len = (uint16_t)header.len;
> +					header.caplen);
> +			mbuf->data_len = (uint16_t)header.caplen;
>   		} else {
>   			/* Try read jumbo frame into multi mbufs. */
>   			if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool,
>   						       mbuf,
>   						       packet,
> -						       header.len) == -1))
> +						       header.caplen) == -1))
>   				break;
>   		}
>   
> -		mbuf->pkt_len = (uint16_t)header.len;
> +		mbuf->pkt_len = (uint16_t)header.caplen;
>   		mbuf->port = pcap_q->in_port;
>   		bufs[num_rx] = mbuf;
>   		num_rx++;
> -		rx_bytes += header.len;
> +		rx_bytes += header.caplen;
>   	}
>   	pcap_q->rx_pkts += num_rx;
>   	pcap_q->rx_bytes += rx_bytes;
  

On Thu, Jan 28, 2016 at 06:14:45PM +0000, Nicolas Pernas Maradei wrote:
> Hi Dror,
> 
> Good catch. What you are saying makes sense and it is also explained in
> pcap's documentation. Was your setup unusual though?
> This might sound like a silly question but I don't remember seeing that
> issue and I should have since your fix is correct.
> 
> Nico.
> 

Hi Nico,

I assume from your reply that you are ok with this patch. Can you please confirm 
this by acking it using the proper form "Acked-by: " line.

Thanks,
/Bruce
  

Hi,

I'm just adding the Acked-by line to the patch. Apologise I missed that one earlier.

Nico.


On 28/01/16 11:09, Dror Birkman wrote:
> The actual captured length is header.caplen, whereas header.len is
> the original length on the wire.
>
> Signed-off-by: Dror Birkman <dror.birkman@lightcyber.com>
> Acked-by: Nicolas Pernas Maradei <nicolas.pernas.maradei@emutex.com>
> ---
>
>
> Without this fix, if the captured length is smaller than the original
> length on the wire, mbuf will contain incorrect data.
>
>
>  drivers/net/pcap/rte_eth_pcap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index f9230eb..1d121f8 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -220,25 +220,25 @@ eth_pcap_rx(void *queue,
>  		buf_size = (uint16_t)(rte_pktmbuf_data_room_size(pcap_q->mb_pool) -
>  				RTE_PKTMBUF_HEADROOM);
>  
> -		if (header.len <= buf_size) {
> +		if (header.caplen <= buf_size) {
>  			/* pcap packet will fit in the mbuf, go ahead and copy */
>  			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
> -					header.len);
> -			mbuf->data_len = (uint16_t)header.len;
> +					header.caplen);
> +			mbuf->data_len = (uint16_t)header.caplen;
>  		} else {
>  			/* Try read jumbo frame into multi mbufs. */
>  			if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool,
>  						       mbuf,
>  						       packet,
> -						       header.len) == -1))
> +						       header.caplen) == -1))
>  				break;
>  		}
>  
> -		mbuf->pkt_len = (uint16_t)header.len;
> +		mbuf->pkt_len = (uint16_t)header.caplen;
>  		mbuf->port = pcap_q->in_port;
>  		bufs[num_rx] = mbuf;
>  		num_rx++;
> -		rx_bytes += header.len;
> +		rx_bytes += header.caplen;
>  	}
>  	pcap_q->rx_pkts += num_rx;
>  	pcap_q->rx_bytes += rx_bytes;
  

On Thu, Jan 28, 2016 at 06:14:45PM +0000, Nicolas Pernas Maradei wrote:
> Hi Dror,
> 
> Good catch. What you are saying makes sense and it is also explained in
> pcap's documentation. Was your setup unusual though?
> This might sound like a silly question but I don't remember seeing that
> issue and I should have since your fix is correct.
> 
> Nico.
> 
Applied to dpdk-next-net/rel_16_04

/Bruce