Re: [PATCH 1/2 v2] net: hsr: fix incorrect lsdu size in the tag of HSR frames for small frames

[Murali Karicheri]

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@xxxxxx>
> > > ---
> > > Â 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