DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v01] net/af_packet: don't specify protocol on socket create
@ 2024-10-13 13:59 Gur Stavi
  2024-10-16 23:49 ` Ferruh Yigit
  2024-10-17  1:38 ` Stephen Hemminger
  0 siblings, 2 replies; 5+ messages in thread
From: Gur Stavi @ 2024-10-13 13:59 UTC (permalink / raw)
  To: Gur Stavi; +Cc: dev, John W. Linville

When creating AF_PACKET socket with specified protocol it is
immediately implicitly bound to any existing interface and
becomes RUNNING. Calling bind on such socket is affectively unbind
from "any interface", then bind to the specific interface.

When creating socket with 0 as protocol, it is created in non-RUNNING
state, then it can be bound to interface and protocol in a single bind
call and switch to RUNNING state.

Especially with ETH_P_ALL, binding to any interface is not a good idea.
It is safer and faster to use the 2nd approach.

This patch replaces protocol in socket creation from ETH_P_ALL to 0.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 4 ++--
 1 file changed, 2 insertions(+), 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 68870dd3e2..b30f88d618 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -650,7 +650,7 @@ open_packet_iface(const char *key __rte_unused,
 	int *sockfd = extra_args;
 
 	/* Open an AF_PACKET socket... */
-	*sockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
+	*sockfd = socket(AF_PACKET, SOCK_RAW, 0);
 	if (*sockfd == -1) {
 		PMD_LOG(ERR, "Could not open AF_PACKET socket");
 		return -1;
@@ -778,7 +778,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 
 	for (q = 0; q < nb_queues; q++) {
 		/* Open an AF_PACKET socket for this queue... */
-		qsockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
+		qsockfd = socket(AF_PACKET, SOCK_RAW, 0);
 		if (qsockfd == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not open AF_PACKET socket",

base-commit: 98613d32e3dac58d685f4f236cf8cc9733abaaf3
-- 
2.40.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v01] net/af_packet: don't specify protocol on socket create
  2024-10-13 13:59 [PATCH v01] net/af_packet: don't specify protocol on socket create Gur Stavi
@ 2024-10-16 23:49 ` Ferruh Yigit
  2024-10-17  1:38 ` Stephen Hemminger
  1 sibling, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2024-10-16 23:49 UTC (permalink / raw)
  To: Gur Stavi; +Cc: dev, John W. Linville

On 10/13/2024 2:59 PM, Gur Stavi wrote:
> When creating AF_PACKET socket with specified protocol it is
> immediately implicitly bound to any existing interface and
> becomes RUNNING. Calling bind on such socket is affectively unbind
> from "any interface", then bind to the specific interface.
> 

Doesn't mean even if we don't bind the socket, will it receive packets?
And what is "any interface" here, does it mean all active interfaces or
one active interface, like first one?

> When creating socket with 0 as protocol, it is created in non-RUNNING
> state, then it can be bound to interface and protocol in a single bind
> call and switch to RUNNING state.
> 
> Especially with ETH_P_ALL, binding to any interface is not a good idea.
> It is safer and faster to use the 2nd approach.
>

I can see unbinding and binding may add to initialization time but that
is slow path anyway.
Why it is not good idea, or less safe?

> 
> This patch replaces protocol in socket creation from ETH_P_ALL to 0.
> 
> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 4 ++--
>  1 file changed, 2 insertions(+), 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 68870dd3e2..b30f88d618 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -650,7 +650,7 @@ open_packet_iface(const char *key __rte_unused,
>  	int *sockfd = extra_args;
>  
>  	/* Open an AF_PACKET socket... */
> -	*sockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
> +	*sockfd = socket(AF_PACKET, SOCK_RAW, 0);
>  	if (*sockfd == -1) {
>  		PMD_LOG(ERR, "Could not open AF_PACKET socket");
>  		return -1;
> @@ -778,7 +778,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  
>  	for (q = 0; q < nb_queues; q++) {
>  		/* Open an AF_PACKET socket for this queue... */
> -		qsockfd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
> +		qsockfd = socket(AF_PACKET, SOCK_RAW, 0);
>  		if (qsockfd == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not open AF_PACKET socket",
> 
> base-commit: 98613d32e3dac58d685f4f236cf8cc9733abaaf3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v01] net/af_packet: don't specify protocol on socket create
  2024-10-13 13:59 [PATCH v01] net/af_packet: don't specify protocol on socket create Gur Stavi
  2024-10-16 23:49 ` Ferruh Yigit
@ 2024-10-17  1:38 ` Stephen Hemminger
  2024-10-25 23:10   ` Ferruh Yigit
  1 sibling, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2024-10-17  1:38 UTC (permalink / raw)
  To: Gur Stavi; +Cc: dev, John W. Linville

On Sun, 13 Oct 2024 16:59:47 +0300
Gur Stavi <gur.stavi@huawei.com> wrote:

> When creating AF_PACKET socket with specified protocol it is
> immediately implicitly bound to any existing interface and
> becomes RUNNING. Calling bind on such socket is affectively unbind
> from "any interface", then bind to the specific interface.
> 
> When creating socket with 0 as protocol, it is created in non-RUNNING
> state, then it can be bound to interface and protocol in a single bind
> call and switch to RUNNING state.
> 
> Especially with ETH_P_ALL, binding to any interface is not a good idea.
> It is safer and faster to use the 2nd approach.
> 
> This patch replaces protocol in socket creation from ETH_P_ALL to 0.
> 
> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>

This makes sense see packet(7) man page:

       By default, all packets of the specified protocol type are passed  to  a
       packet  socket.   To  get  packets  only  from  a specific interface use
       bind(2) specifying an address in a struct sockaddr_ll to bind the packet
       socket to an interface.  Fields used for binding are sll_family  (should
       be AF_PACKET), sll_protocol, and sll_ifindex.

So there is a small window where the packet socket could pick up junk before
the bind from other interfaces.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v01] net/af_packet: don't specify protocol on socket create
  2024-10-17  1:38 ` Stephen Hemminger
@ 2024-10-25 23:10   ` Ferruh Yigit
  2024-10-25 23:47     ` Ferruh Yigit
  0 siblings, 1 reply; 5+ messages in thread
From: Ferruh Yigit @ 2024-10-25 23:10 UTC (permalink / raw)
  To: Stephen Hemminger, Gur Stavi; +Cc: dev, John W. Linville

On 10/17/2024 2:38 AM, Stephen Hemminger wrote:
> On Sun, 13 Oct 2024 16:59:47 +0300
> Gur Stavi <gur.stavi@huawei.com> wrote:
> 
>> When creating AF_PACKET socket with specified protocol it is
>> immediately implicitly bound to any existing interface and
>> becomes RUNNING. Calling bind on such socket is affectively unbind
>> from "any interface", then bind to the specific interface.
>>
>> When creating socket with 0 as protocol, it is created in non-RUNNING
>> state, then it can be bound to interface and protocol in a single bind
>> call and switch to RUNNING state.
>>
>> Especially with ETH_P_ALL, binding to any interface is not a good idea.
>> It is safer and faster to use the 2nd approach.
>>
>> This patch replaces protocol in socket creation from ETH_P_ALL to 0.
>>
>> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
> 
> This makes sense see packet(7) man page:
> 
>        By default, all packets of the specified protocol type are passed  to  a
>        packet  socket.   To  get  packets  only  from  a specific interface use
>        bind(2) specifying an address in a struct sockaddr_ll to bind the packet
>        socket to an interface.  Fields used for binding are sll_family  (should
>        be AF_PACKET), sll_protocol, and sll_ifindex.
> 
> So there is a small window where the packet socket could pick up junk before
> the bind from other interfaces.
>

Thanks, this answers some of my questions in this thread, also I did
some experiment and verified the same.

I agree this change is more close the intention of the driver (driver is
not to get packets from all interfaces), hence:

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v01] net/af_packet: don't specify protocol on socket create
  2024-10-25 23:10   ` Ferruh Yigit
@ 2024-10-25 23:47     ` Ferruh Yigit
  0 siblings, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2024-10-25 23:47 UTC (permalink / raw)
  To: Stephen Hemminger, Gur Stavi; +Cc: dev, John W. Linville

On 10/26/2024 12:10 AM, Ferruh Yigit wrote:
> On 10/17/2024 2:38 AM, Stephen Hemminger wrote:
>> On Sun, 13 Oct 2024 16:59:47 +0300
>> Gur Stavi <gur.stavi@huawei.com> wrote:
>>
>>> When creating AF_PACKET socket with specified protocol it is
>>> immediately implicitly bound to any existing interface and
>>> becomes RUNNING. Calling bind on such socket is affectively unbind
>>> from "any interface", then bind to the specific interface.
>>>
>>> When creating socket with 0 as protocol, it is created in non-RUNNING
>>> state, then it can be bound to interface and protocol in a single bind
>>> call and switch to RUNNING state.
>>>
>>> Especially with ETH_P_ALL, binding to any interface is not a good idea.
>>> It is safer and faster to use the 2nd approach.
>>>
>>> This patch replaces protocol in socket creation from ETH_P_ALL to 0.
>>>
>>> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
>>
>> This makes sense see packet(7) man page:
>>
>>        By default, all packets of the specified protocol type are passed  to  a
>>        packet  socket.   To  get  packets  only  from  a specific interface use
>>        bind(2) specifying an address in a struct sockaddr_ll to bind the packet
>>        socket to an interface.  Fields used for binding are sll_family  (should
>>        be AF_PACKET), sll_protocol, and sll_ifindex.
>>
>> So there is a small window where the packet socket could pick up junk before
>> the bind from other interfaces.
>>
> 
> Thanks, this answers some of my questions in this thread, also I did
> some experiment and verified the same.
> 
> I agree this change is more close the intention of the driver (driver is
> not to get packets from all interfaces), hence:
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 

Series applied to dpdk-next-net/main, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-10-25 23:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-13 13:59 [PATCH v01] net/af_packet: don't specify protocol on socket create Gur Stavi
2024-10-16 23:49 ` Ferruh Yigit
2024-10-17  1:38 ` Stephen Hemminger
2024-10-25 23:10   ` Ferruh Yigit
2024-10-25 23:47     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).