[net PATCH] macvlan: Only deliver one copy of the frame to the macvlan interface

[net PATCH] macvlan: Only deliver one copy of the frame to the macvlan interface

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch intoduces a slight adjustment for macvlan to address the fact
that in source mode I was seeing two copies of any packet addressed to the
macvlan interface being delivered where there should have been only one.

The issue appears to be that one copy was delivered based on the source MAC
address and then the second copy was being delivered based on the
destination MAC address. To fix it I am just freeing the second copy
instead of delivering it up the stack using the same netdev as was already
delivered to.

Fixes: 79cf79abce71 ("macvlan: add source mode")
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/macvlan.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d2aea961e0f4..744b0fe6dc78 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -484,7 +484,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 		return RX_HANDLER_PASS;
 
 	dev = vlan->dev;
-	if (unlikely(!(dev->flags & IFF_UP))) {
+	if ((vlan->mode == MACVLAN_MODE_SOURCE) ||
+	    unlikely(!(dev->flags & IFF_UP))) {
 		kfree_skb(skb);
 		return RX_HANDLER_CONSUMED;
 	}

Re: [net PATCH] macvlan: Only deliver one copy of the frame to the macvlan interface

[Eric Dumazet]

On Sun, 2017-10-08 at 15:54 -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> This patch intoduces a slight adjustment for macvlan to address the fact
> that in source mode I was seeing two copies of any packet addressed to the
> macvlan interface being delivered where there should have been only one.
> 
> The issue appears to be that one copy was delivered based on the source MAC
> address and then the second copy was being delivered based on the
> destination MAC address. To fix it I am just freeing the second copy
> instead of delivering it up the stack using the same netdev as was already
> delivered to.
> 
> Fixes: 79cf79abce71 ("macvlan: add source mode")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  drivers/net/macvlan.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index d2aea961e0f4..744b0fe6dc78 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -484,7 +484,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>  		return RX_HANDLER_PASS;
>  
>  	dev = vlan->dev;
> -	if (unlikely(!(dev->flags & IFF_UP))) {
> +	if ((vlan->mode == MACVLAN_MODE_SOURCE) ||
> +	    unlikely(!(dev->flags & IFF_UP))) {
>  		kfree_skb(skb);
>  		return RX_HANDLER_CONSUMED;
>  	}
> 


Shouldn't we have a consume_skb() then instead of kfree_skb() ?

We are not really dropping a packet here, only avoiding some artifact
cause by the cited commit.

Re: [net PATCH] macvlan: Only deliver one copy of the frame to the macvlan interface

[Alexander Duyck]

On Sun, Oct 8, 2017 at 6:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2017-10-08 at 15:54 -0700, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> This patch intoduces a slight adjustment for macvlan to address the fact
>> that in source mode I was seeing two copies of any packet addressed to the
>> macvlan interface being delivered where there should have been only one.
>>
>> The issue appears to be that one copy was delivered based on the source MAC
>> address and then the second copy was being delivered based on the
>> destination MAC address. To fix it I am just freeing the second copy
>> instead of delivering it up the stack using the same netdev as was already
>> delivered to.
>>
>> Fixes: 79cf79abce71 ("macvlan: add source mode")
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>  drivers/net/macvlan.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index d2aea961e0f4..744b0fe6dc78 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -484,7 +484,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>>               return RX_HANDLER_PASS;
>>
>>       dev = vlan->dev;
>> -     if (unlikely(!(dev->flags & IFF_UP))) {
>> +     if ((vlan->mode == MACVLAN_MODE_SOURCE) ||
>> +         unlikely(!(dev->flags & IFF_UP))) {
>>               kfree_skb(skb);
>>               return RX_HANDLER_CONSUMED;
>>       }
>>
>
>
> Shouldn't we have a consume_skb() then instead of kfree_skb() ?
>
> We are not really dropping a packet here, only avoiding some artifact
> cause by the cited commit.

The cited commit basically introduced an issue where we are cloning it
and sending the clone to the correct device and then are stuck with
the original. The way I fixed it is currently consistent with how
broadcast is already being handled for macvlan since they are calling
kfree_skb() on the clone that they end up enqueueing for broadcast.

My thought is to look at rewriting this in relation to some other work
I am doing, but I wanted to have a fix for net and stable kernels that
prevents this frame duplication from occurring. Really in order to
handle this correctly my thought is that we should probably be doing a
vlan_prev similar to how we have a pt_prev in
__netif_receive_skb_core. Then that way when a packet is meant to be
handled by one interface, as is the case for most unicast traffic with
VLAN regardless of source mode or not we can then just jump back in
using RX_HANDLER_ANOTHER.

- Alex

Re: [net PATCH] macvlan: Only deliver one copy of the frame to the macvlan interface

[Alexander Duyck]

On Mon, Oct 9, 2017 at 10:30 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Sun, Oct 8, 2017 at 6:07 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Sun, 2017-10-08 at 15:54 -0700, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>>
>>> This patch intoduces a slight adjustment for macvlan to address the fact
>>> that in source mode I was seeing two copies of any packet addressed to the
>>> macvlan interface being delivered where there should have been only one.
>>>
>>> The issue appears to be that one copy was delivered based on the source MAC
>>> address and then the second copy was being delivered based on the
>>> destination MAC address. To fix it I am just freeing the second copy
>>> instead of delivering it up the stack using the same netdev as was already
>>> delivered to.
>>>
>>> Fixes: 79cf79abce71 ("macvlan: add source mode")
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>>> ---
>>>  drivers/net/macvlan.c |    3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>>> index d2aea961e0f4..744b0fe6dc78 100644
>>> --- a/drivers/net/macvlan.c
>>> +++ b/drivers/net/macvlan.c
>>> @@ -484,7 +484,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>>>               return RX_HANDLER_PASS;
>>>
>>>       dev = vlan->dev;
>>> -     if (unlikely(!(dev->flags & IFF_UP))) {
>>> +     if ((vlan->mode == MACVLAN_MODE_SOURCE) ||
>>> +         unlikely(!(dev->flags & IFF_UP))) {
>>>               kfree_skb(skb);
>>>               return RX_HANDLER_CONSUMED;
>>>       }
>>>
>>
>>
>> Shouldn't we have a consume_skb() then instead of kfree_skb() ?
>>
>> We are not really dropping a packet here, only avoiding some artifact
>> cause by the cited commit.
>
> The cited commit basically introduced an issue where we are cloning it
> and sending the clone to the correct device and then are stuck with
> the original. The way I fixed it is currently consistent with how
> broadcast is already being handled for macvlan since they are calling
> kfree_skb() on the clone that they end up enqueueing for broadcast.
>
> My thought is to look at rewriting this in relation to some other work
> I am doing, but I wanted to have a fix for net and stable kernels that
> prevents this frame duplication from occurring. Really in order to
> handle this correctly my thought is that we should probably be doing a
> vlan_prev similar to how we have a pt_prev in
> __netif_receive_skb_core. Then that way when a packet is meant to be
> handled by one interface, as is the case for most unicast traffic with
> VLAN regardless of source mode or not we can then just jump back in
> using RX_HANDLER_ANOTHER.
>
> - Alex

Actually, now that I am thinking it over again maybe us calling
kfree_skb() isn't the correct answer. It might make more sense to just
return RX_HANDLER_PASS. Then we can defer it to the original interface
to drop it.

- Alex