diff mbox series

[v2,1/3] net/af_packet: set_mtu() decrements sockaddr twice

Message ID 1542709592-215007-1-git-send-email-tiago.lam@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v2,1/3] net/af_packet: set_mtu() decrements sockaddr twice |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Lam, Tiago Nov. 20, 2018, 10:26 a.m. UTC 
  When setting the MTU, eth_dev_mtu_set() is called to validate the
provided MTU. As part of that, it calculates the useful area to store
data and compares it against the MTU, to guarantee that there's enough
space to store the data. It calculates that as:
    "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"

However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
sockaddr_ll) internally, meaning the useuful area of data above will
have sizeof(struct sockaddr_ll) decremented twice.

Instead, the useful area of data should be calculated as:
    "tp_frame_size - TPACKET2_HDRLEN"

This makes sure that there's enough useful area to fit the provided MTU
after excluding tpacket2_hdr and sockaddr_ll.

Fixes: cc68ac4 ("net/af_packet: support MTU change")

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Kevin Traynor Nov. 20, 2018, 10:29 a.m. UTC | #1 
On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.
> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")
> 

It should be for stable also?

> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 95a98c6..264cfc0 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>  	int ret;
>  	int s;
>  	unsigned int data_size = internals->req.tp_frame_size -
> -				 TPACKET2_HDRLEN -
> -				 sizeof(struct sockaddr_ll);
> +				 TPACKET2_HDRLEN;
>  
>  	if (mtu > data_size)
>  		return -EINVAL;
>
Lam, Tiago Nov. 20, 2018, 10:45 a.m. UTC | #2 
On 20/11/2018 10:29, Kevin Traynor wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> When setting the MTU, eth_dev_mtu_set() is called to validate the
>> provided MTU. As part of that, it calculates the useful area to store
>> data and compares it against the MTU, to guarantee that there's enough
>> space to store the data. It calculates that as:
>>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
>>
>> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
>> sockaddr_ll) internally, meaning the useuful area of data above will
>> have sizeof(struct sockaddr_ll) decremented twice.
>>
>> Instead, the useful area of data should be calculated as:
>>     "tp_frame_size - TPACKET2_HDRLEN"
>>
>> This makes sure that there's enough useful area to fit the provided MTU
>> after excluding tpacket2_hdr and sockaddr_ll.
>>
>> Fixes: cc68ac4 ("net/af_packet: support MTU change")
>>
> 
> It should be for stable also?

Indeed, thanks for pointing that out. I've missed the Cc there, but it
should be backported - I can track it down to 17.02. I'll make sure I
add the Cc if a new iteration is needed.

Tiago.

> 
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>> ---
>>  drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
>> index 95a98c6..264cfc0 100644
>> --- a/drivers/net/af_packet/rte_eth_af_packet.c
>> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
>> @@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>>  	int ret;
>>  	int s;
>>  	unsigned int data_size = internals->req.tp_frame_size -
>> -				 TPACKET2_HDRLEN -
>> -				 sizeof(struct sockaddr_ll);
>> +				 TPACKET2_HDRLEN;
>>  
>>  	if (mtu > data_size)
>>  		return -EINVAL;
>>
>
Ferruh Yigit Nov. 27, 2018, 5:42 p.m. UTC | #3 
On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.

There are a few typos above.

> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")

The syntax is slightly different, can you please try following git alias:
alias.fixline=log -1 --abbrev=12 --format='Fixes: %h ("%s")%nCc: %ae'

> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>

Also can you please fix ./devtools/check-git-log.sh warnings on patches?
Ferruh Yigit Dec. 21, 2018, 12:29 p.m. UTC | #4 
On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.
> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")
> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 95a98c6..264cfc0 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -433,8 +433,7 @@  eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	int ret;
 	int s;
 	unsigned int data_size = internals->req.tp_frame_size -
-				 TPACKET2_HDRLEN -
-				 sizeof(struct sockaddr_ll);
+				 TPACKET2_HDRLEN;
 
 	if (mtu > data_size)
 		return -EINVAL;