netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
@ 2020-07-17 14:55 Murali Karicheri
  2020-07-17 14:55 ` [PATCH 2/2 v2] net: hsr: validate address B before copying to skb Murali Karicheri
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Murali Karicheri @ 2020-07-17 14:55 UTC (permalink / raw)
  To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko,
	vinicius.gomes

For small Ethernet frames with size less than minimum size 66 for HSR
vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
frame to make it minimum size. This results in incorrect LSDU size being
populated in the HSR tag for these frames. Fix this by padding the frame
to the minimum size applicable for HSR.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 no change from original version
 Sending this bug fix ahead of PRP patch series as per comment
 net/hsr/hsr_forward.c | 3 +++
 1 file changed, 3 insertions(+)

 Sending this bug fix ahead of PRP patch series as per comment
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ed13760463de..e42fd356f073 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -127,6 +127,9 @@ static void hsr_fill_tag(struct sk_buff *skb, struct hsr_frame_info *frame,
 	int lane_id;
 	int lsdu_size;
 
+	/* pad to minimum packet size which is 60 + 6 (HSR tag) */
+	skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);
+
 	if (port->type == HSR_PT_SLAVE_A)
 		lane_id = 0;
 	else
-- 
2.17.1


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

* [PATCH 2/2 v2] net: hsr: validate address B before copying to skb
  2020-07-17 14:55 [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Murali Karicheri
@ 2020-07-17 14:55 ` Murali Karicheri
  2020-07-18  1:54   ` David Miller
  2020-07-17 17:39 ` [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Grygorii Strashko
  2020-07-18  1:54 ` David Miller
  2 siblings, 1 reply; 7+ messages in thread
From: Murali Karicheri @ 2020-07-17 14:55 UTC (permalink / raw)
  To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko,
	vinicius.gomes

Validate MAC address before copying the same to outgoing frame
skb destination address. Since a node can have zero mac
address for Link B until a valid frame is received over
that link, this fix address the issue of a zero MAC address
being in the packet.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
 Removing prp prefix from subject line.
 Sending this bug fix ahead of PRP patch series as per comment
 net/hsr/hsr_framereg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 03b891904314..530de24b1fb5 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -325,7 +325,8 @@ void hsr_addr_subst_dest(struct hsr_node *node_src, struct sk_buff *skb,
 	if (port->type != node_dst->addr_B_port)
 		return;
 
-	ether_addr_copy(eth_hdr(skb)->h_dest, node_dst->macaddress_B);
+	if (is_valid_ether_addr(node_dst->macaddress_B))
+		ether_addr_copy(eth_hdr(skb)->h_dest, node_dst->macaddress_B);
 }
 
 void hsr_register_frame_in(struct hsr_node *node, struct hsr_port *port,
-- 
2.17.1


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

* Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
  2020-07-17 14:55 [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Murali Karicheri
  2020-07-17 14:55 ` [PATCH 2/2 v2] net: hsr: validate address B before copying to skb Murali Karicheri
@ 2020-07-17 17:39 ` Grygorii Strashko
  2020-07-20 14:08   ` Murali Karicheri
  2020-07-18  1:54 ` David Miller
  2 siblings, 1 reply; 7+ messages in thread
From: Grygorii Strashko @ 2020-07-17 17:39 UTC (permalink / raw)
  To: Murali Karicheri, davem, kuba, netdev, linux-kernel, nsekhar,
	vinicius.gomes



On 17/07/2020 17:55, Murali Karicheri wrote:
> For small Ethernet frames with size less than minimum size 66 for HSR
> vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
> frame to make it minimum size. This results in incorrect LSDU size being
> populated in the HSR tag for these frames. Fix this by padding the frame
> to the minimum size applicable for HSR.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
>   no change from original version
>   Sending this bug fix ahead of PRP patch series as per comment
>   net/hsr/hsr_forward.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
>   Sending this bug fix ahead of PRP patch series as per comment
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index ed13760463de..e42fd356f073 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -127,6 +127,9 @@ static void hsr_fill_tag(struct sk_buff *skb, struct hsr_frame_info *frame,
>   	int lane_id;
>   	int lsdu_size;
>   
> +	/* pad to minimum packet size which is 60 + 6 (HSR tag) */
> +	skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);

It may fail.
And i worry that it might be not the right place to do that
(if packet is small it will be called for every copy of the packet).
May be it has to be done once when packet enters LRE device?

> +
>   	if (port->type == HSR_PT_SLAVE_A)
>   		lane_id = 0;
>   	else
> 

-- 
Best regards,
grygorii

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

* Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
  2020-07-17 14:55 [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Murali Karicheri
  2020-07-17 14:55 ` [PATCH 2/2 v2] net: hsr: validate address B before copying to skb Murali Karicheri
  2020-07-17 17:39 ` [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Grygorii Strashko
@ 2020-07-18  1:54 ` David Miller
  2 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2020-07-18  1:54 UTC (permalink / raw)
  To: m-karicheri2
  Cc: kuba, netdev, linux-kernel, nsekhar, grygorii.strashko, vinicius.gomes

From: Murali Karicheri <m-karicheri2@ti.com>
Date: Fri, 17 Jul 2020 10:55:09 -0400

> For small Ethernet frames with size less than minimum size 66 for HSR
> vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
> frame to make it minimum size. This results in incorrect LSDU size being
> populated in the HSR tag for these frames. Fix this by padding the frame
> to the minimum size applicable for HSR.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Applied.

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

* Re: [PATCH 2/2 v2] net: hsr: validate address B before copying to skb
  2020-07-17 14:55 ` [PATCH 2/2 v2] net: hsr: validate address B before copying to skb Murali Karicheri
@ 2020-07-18  1:54   ` David Miller
  0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2020-07-18  1:54 UTC (permalink / raw)
  To: m-karicheri2
  Cc: kuba, netdev, linux-kernel, nsekhar, grygorii.strashko, vinicius.gomes

From: Murali Karicheri <m-karicheri2@ti.com>
Date: Fri, 17 Jul 2020 10:55:10 -0400

> Validate MAC address before copying the same to outgoing frame
> skb destination address. Since a node can have zero mac
> address for Link B until a valid frame is received over
> that link, this fix address the issue of a zero MAC address
> being in the packet.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Applied.

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

* Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
  2020-07-17 17:39 ` [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Grygorii Strashko
@ 2020-07-20 14:08   ` Murali Karicheri
  2020-07-20 15:25     ` Murali Karicheri
  0 siblings, 1 reply; 7+ messages in thread
From: Murali Karicheri @ 2020-07-20 14:08 UTC (permalink / raw)
  To: Grygorii Strashko, davem, kuba, netdev, linux-kernel, nsekhar,
	vinicius.gomes

Grygorii,

On 7/17/20 1:39 PM, Grygorii Strashko wrote:
> 
> 
> On 17/07/2020 17:55, Murali Karicheri wrote:
>> For small Ethernet frames with size less than minimum size 66 for HSR
>> vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
>> frame to make it minimum size. This results in incorrect LSDU size being
>> populated in the HSR tag for these frames. Fix this by padding the frame
>> to the minimum size applicable for HSR.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> ---
>>   no change from original version
>>   Sending this bug fix ahead of PRP patch series as per comment
>>   net/hsr/hsr_forward.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>>   Sending this bug fix ahead of PRP patch series as per comment
>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>> index ed13760463de..e42fd356f073 100644
>> --- a/net/hsr/hsr_forward.c
>> +++ b/net/hsr/hsr_forward.c
>> @@ -127,6 +127,9 @@ static void hsr_fill_tag(struct sk_buff *skb, 
>> struct hsr_frame_info *frame,
>>       int lane_id;
>>       int lsdu_size;
>> +    /* pad to minimum packet size which is 60 + 6 (HSR tag) */
>> +    skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);
> 
> It may fail.
> And i worry that it might be not the right place to do that
> (if packet is small it will be called for every copy of the packet).
> May be it has to be done once when packet enters LRE device?
> 
A better place may be to add it at the beginning of
hsr_fill_frame_info() at which point there is one copy and after that
code enters hsr_forward_do() to replicate. I don't think we can place it
anywhere before that code.

hsr_dev_xmit()
    - hsr_forward_skb()
       - hsr_fill_frame_info()

Inside hsr_fill_frame_info() we could do

	if (ethhdr->h_proto == htons(ETH_P_8021Q)) {
		frame->is_vlan = true;
		/* FIXME: */
		netdev_warn_once(skb->dev, "VLAN not yet supported");
	}
+	min_size = ETH_ZLEN + HSR_HLEN;
+       if (frame->is_vlan)
+		min_size += 4;
+	ret = skb_put_padto(skb, min_size))
+	if (ret)
+	     return ret;

At this point, it will be ready to tag the frame. Frame will be either a
supervision frame which is already tagged or standard frame from upper
layer. Either case, padto() is required. So looks like the right place
to avoid doing it twice.

And packet would get dropped at the caller if skb_put_padto() fails. So
we could return the return value to the caller.

This also eliminates similar padto() call in 
send_hsr_supervision_frame() as well.

What do you think?

Murali
>> +
>>       if (port->type == HSR_PT_SLAVE_A)
>>           lane_id = 0;
>>       else
>>
> 

-- 
Murali Karicheri
Texas Instruments

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

* Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames
  2020-07-20 14:08   ` Murali Karicheri
@ 2020-07-20 15:25     ` Murali Karicheri
  0 siblings, 0 replies; 7+ messages in thread
From: Murali Karicheri @ 2020-07-20 15:25 UTC (permalink / raw)
  To: Grygorii Strashko, davem, kuba, netdev, linux-kernel, nsekhar,
	vinicius.gomes

Hi Grygoii,

On 7/20/20 10:08 AM, Murali Karicheri wrote:
> Grygorii,
> 
> On 7/17/20 1:39 PM, Grygorii Strashko wrote:
>>
>>
>> On 17/07/2020 17:55, Murali Karicheri wrote:
>>> For small Ethernet frames with size less than minimum size 66 for HSR
>>> vs 60 for regular Ethernet frames, hsr driver currently doesn't pad the
>>> frame to make it minimum size. This results in incorrect LSDU size being
>>> populated in the HSR tag for these frames. Fix this by padding the frame
>>> to the minimum size applicable for HSR.
>>>
>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>> ---
>>>   no change from original version
>>>   Sending this bug fix ahead of PRP patch series as per comment
>>>   net/hsr/hsr_forward.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>>   Sending this bug fix ahead of PRP patch series as per comment
>>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>>> index ed13760463de..e42fd356f073 100644
>>> --- a/net/hsr/hsr_forward.c
>>> +++ b/net/hsr/hsr_forward.c
>>> @@ -127,6 +127,9 @@ static void hsr_fill_tag(struct sk_buff *skb, 
>>> struct hsr_frame_info *frame,
>>>       int lane_id;
>>>       int lsdu_size;
>>> +    /* pad to minimum packet size which is 60 + 6 (HSR tag) */
>>> +    skb_put_padto(skb, ETH_ZLEN + HSR_HLEN);
>>
>> It may fail.
>> And i worry that it might be not the right place to do that
>> (if packet is small it will be called for every copy of the packet).
>> May be it has to be done once when packet enters LRE device?
>>
> A better place may be to add it at the beginning of
> hsr_fill_frame_info() at which point there is one copy and after that
> code enters hsr_forward_do() to replicate. I don't think we can place it
> anywhere before that code.
> 
> hsr_dev_xmit()
>     - hsr_forward_skb()
>        - hsr_fill_frame_info()
> 
> Inside hsr_fill_frame_info() we could do
> 
>      if (ethhdr->h_proto == htons(ETH_P_8021Q)) {
>          frame->is_vlan = true;
>          /* FIXME: */
>          netdev_warn_once(skb->dev, "VLAN not yet supported");
>      }
> +    min_size = ETH_ZLEN + HSR_HLEN;
> +       if (frame->is_vlan)
> +        min_size += 4;
> +    ret = skb_put_padto(skb, min_size))
> +    if (ret)
> +         return ret;
> 
> At this point, it will be ready to tag the frame. Frame will be either a
> supervision frame which is already tagged or standard frame from upper
> layer. Either case, padto() is required. So looks like the right place
> to avoid doing it twice.
> 
> And packet would get dropped at the caller if skb_put_padto() fails. So
> we could return the return value to the caller.
> 
> This also eliminates similar padto() call in 
> send_hsr_supervision_frame() as well.
> 
> What do you think?

Dave has already applied the patch. So I will send a follow up
patch that fixes the original issue to check for the return type.

The patch to optimize this at the correct location (second part
of your concern) can be a separate patch that will send as a follow up
to the PRP series as that code touches this as well. So better to
optimize this after the PRP series IMO.

I plan to fix similar issue in the PRP series as well ( not
checking for return type) and re-spin. Don't want to delay merge
of PRP series for this optimization that can be addressed
separately as I have said.

Murali
> 
> Murali
>>> +
>>>       if (port->type == HSR_PT_SLAVE_A)
>>>           lane_id = 0;
>>>       else
>>>
>>
> 

-- 
Murali Karicheri
Texas Instruments

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

end of thread, other threads:[~2020-07-20 15:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 14:55 [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Murali Karicheri
2020-07-17 14:55 ` [PATCH 2/2 v2] net: hsr: validate address B before copying to skb Murali Karicheri
2020-07-18  1:54   ` David Miller
2020-07-17 17:39 ` [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames Grygorii Strashko
2020-07-20 14:08   ` Murali Karicheri
2020-07-20 15:25     ` Murali Karicheri
2020-07-18  1:54 ` David Miller

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).