Re: [Linuxptp-devel] [PATCH 1/1] Strip PRP trailer
> > > > + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + > > > > + (cnt > > > > + - 2))); > > > > > > This seems to be parsing the frame from the end. Couldn't that > > > randomly match a non-PRP frame, even if you consider the length > > > check > > below? > > > > > > To me it would make more sense to start after the PTP message > > > according to the messageLength field. > > > > > > > This is a good point. > > PRP supports also pure ethernet frames but in case of PTP over > > ethernet we actually have the messageLength. > > I will need to see if I can get it in a good way already in the raw > > receive (seems like only the ethernet header is currently parsed) > > > > If I understand correctly, the message length is validated in msg.c:429. > How would you feel about a solution where I move the PRP trailer validation > to be just before that check? > > Do you prefer the acceptance of PRP trailer to be active all the time or is it > better if I add another config parameter? > > Side note 1: > The PRP trailer should normally be removed by the PRP implementation, so if > a PRP hw implementation is used the trailer won't be there. > In short, PRP uses dual lines to send the same information, LAN A and LAN B, > the first arriving packet is handled and the duplicate from the other > interface > is dropped. > PTP over PRP can't mix the information from the 2 lines (path delay might be > different) so we need to run a PTP instance on each interface instead of on > the combined prp0 created by the kernel. > Hence the PRP trailer will be still there in PTP packages received directly > from > the interface, the PRP stack hasn't had the chance to remove it (which it > would if traffic is read from prp0) > > Side note 2: > PRP has a requirement for minimum packet length to be 70 bytes so short > messages like e.g. Sync Message (44) would get 2 bytes padding before the 6 > bytes PRP trailer. The LSDU size inside the PRP trailer would be 52 (44 + 2 + > 6). > The 2nd side note is causing a problem when using the message length for validation. In raw receive the information about if VLAN is being used or not, is available. This information is needed in order to know if padding is expected or not in order to meet the minimum 70bytes frame requirement. I think the cleanest solution is to handle the PRP trailer stripping in raw receive. There the information about VLAN is available and it makes the PRP trailer stripping (which normally should be done by PRP stack) invisible to the rest of the PTP implementation. Do you prefer to keep the raw receive clean of PTP message struct or should I make an attempt to parse a PTP message there and use the messageLength field? -- Magnus Armholt ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] Strip PRP trailer
On Thu, Jun 30, 2022 at 08:15:26AM +, Magnus Armholt wrote: > If I understand correctly, the message length is validated in msg.c:429. > How would you feel about a solution where I move the PRP trailer validation > to be > just before that check? To me it would make sense to keep it in the transport-specific code and active all time, but you should wait for Richard's opinion. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] Strip PRP trailer
> > > + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + (cnt > > > + - 2))); > > > > This seems to be parsing the frame from the end. Couldn't that > > randomly match a non-PRP frame, even if you consider the length check > below? > > > > To me it would make more sense to start after the PTP message > > according to the messageLength field. > > > > This is a good point. > PRP supports also pure ethernet frames but in case of PTP over ethernet we > actually have the messageLength. > I will need to see if I can get it in a good way already in the raw receive > (seems like only the ethernet header is currently parsed) > If I understand correctly, the message length is validated in msg.c:429. How would you feel about a solution where I move the PRP trailer validation to be just before that check? Do you prefer the acceptance of PRP trailer to be active all the time or is it better if I add another config parameter? Side note 1: The PRP trailer should normally be removed by the PRP implementation, so if a PRP hw implementation is used the trailer won't be there. In short, PRP uses dual lines to send the same information, LAN A and LAN B, the first arriving packet is handled and the duplicate from the other interface is dropped. PTP over PRP can't mix the information from the 2 lines (path delay might be different) so we need to run a PTP instance on each interface instead of on the combined prp0 created by the kernel. Hence the PRP trailer will be still there in PTP packages received directly from the interface, the PRP stack hasn't had the chance to remove it (which it would if traffic is read from prp0) Side note 2: PRP has a requirement for minimum packet length to be 70 bytes so short messages like e.g. Sync Message (44) would get 2 bytes padding before the 6 bytes PRP trailer. The LSDU size inside the PRP trailer would be 52 (44 + 2 + 6). ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] Strip PRP trailer
Thanks for taking the time to review the patch!
> > +static int has_prp_trailer(unsigned char *ptr, int cnt) {
> > + if (cnt < PRP_TRAILER_LEN)
> > + return -1;
> > + //verify suffix first since it is the best identifier
>
> Please use C-style comments.
>
Will do
> > + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + (cnt -
> > + 2)));
>
> This seems to be parsing the frame from the end. Couldn't that randomly
> match a non-PRP frame, even if you consider the length check below?
>
> To me it would make more sense to start after the PTP message according to
> the messageLength field.
>
This is a good point.
PRP supports also pure ethernet frames but in case of PTP over ethernet we
actually have the messageLength.
I will need to see if I can get it in a good way already in the raw receive
(seems like only the ethernet header is currently parsed)
> > + if (suffix_id != ETH_P_PRP)
> > + return -1;
> > +
> > + // size should also be verified
> > + unsigned short lane_size_field = ntohs(*(unsigned short*)(ptr +
> > + (cnt - 4)));
>
> Declarations should be at the beginning of the function.
>
Ok, impressive that you still support such old c-style, thanks for pointing it
out.
> > + // size is lower 12 bits
> > + unsigned short lsdu_size = (lane_size_field & 0x0FFF);
> > + if (lsdu_size == cnt)
> > + return 0;
> > +
> > + return -1;
> > +}
> > +
>
- Magnus Armholt
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH 1/1] Strip PRP trailer
On Wed, Jun 29, 2022 at 03:28:54PM +0300, Magnus Armholt wrote:
> Strip the IEC62439-3 PRP trailer if it is present
> to support PTP over PRP.
> The implementation is very pedantic about
> trailing bytes and will indicate bad message if the
> PRP trailer bytes are present when parsing the PTP message.
> +static int has_prp_trailer(unsigned char *ptr, int cnt)
> +{
> + if (cnt < PRP_TRAILER_LEN)
> + return -1;
> + //verify suffix first since it is the best identifier
Please use C-style comments.
> + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + (cnt - 2)));
This seems to be parsing the frame from the end. Couldn't that
randomly match a non-PRP frame, even if you consider the length check
below?
To me it would make more sense to start after the PTP message
according to the messageLength field.
> + if (suffix_id != ETH_P_PRP)
> + return -1;
> +
> + // size should also be verified
> + unsigned short lane_size_field = ntohs(*(unsigned short*)(ptr + (cnt -
> 4)));
Declarations should be at the beginning of the function.
> + // size is lower 12 bits
> + unsigned short lsdu_size = (lane_size_field & 0x0FFF);
> + if (lsdu_size == cnt)
> + return 0;
> +
> + return -1;
> +}
> +
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] PATCH[1/1] Strip PRP trailer
>
>
> > From b6b73ad37d9ca8907feff218b11325fa9456b3eb Mon Sep 17 00:00:00 2001
> > From: Magnus Armholt
> > Date: Wed, 29 Jun 2022 10:00:54 +0300
> > Subject: [PATCH] Strip PRP trailer
> >
> > Strip the IEC62439-3 PRP trailer if it is present.
> > The implementation is very pedantic about
> > trailing bytes and will indicate bad message if the
> > PRP trailer bytes are present when parsing the PTP message.
>
Could you explain why we need this?
Why do we need the "*Parallel Redundancy Protocol* (*PRP*)"?
What are your user cases that use the PRP?
Please send the patch with
git send-email
https://git-scm.com/docs/git-send-email
With your proper
git format-patch
Erez
>
> > Signed-off-by: Magnus Armholt
> > ---
> > raw.c | 24
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/raw.c b/raw.c
> > index ce64684..fb23a1f 100644
> > --- a/raw.c
> > +++ b/raw.c
> > @@ -65,6 +65,8 @@ struct raw {
> > #define N_RAW_FILTER12
> > #define RAW_FILTER_TEST 9
> >
> > +#define PRP_TRAILER_LEN 6
> > +
> > static struct sock_filter raw_filter[N_RAW_FILTER] = {
> > {OP_LDH, 0, 0, OFF_ETYPE },
> > {OP_JEQ, 0, 4, ETH_P_8021Q }, /*f goto non-vlan block*/
> > @@ -206,6 +208,25 @@ static void addr_to_mac(void *mac, struct address
> *addr)
> > memcpy(mac, >sll.sll_addr, MAC_LEN);
> > }
> >
> > +static int has_prp_trailer(unsigned char *ptr, int cnt)
> > +{
> > + if (cnt < PRP_TRAILER_LEN)
> > + return -1;
> > + //verify suffix first since it is the best identifier
> > + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + (cnt - 2)));
> > + if (suffix_id != ETH_P_PRP)
> > + return -1;
> > +
> > + // size should also be verified
> > + unsigned short lane_size_field = ntohs(*(unsigned short*)(ptr + (cnt -
> 4)));
> > + // size is lower 12 bits
> > + unsigned short lsdu_size = (lane_size_field & 0x0FFF);
> > + if (lsdu_size == cnt)
> > + return 0;
> > +
> > + return -1;
> > +}
> > +
> > static int raw_open(struct transport *t, struct interface *iface,
> > struct fdarray *fda, enum timestamp_type ts_type)
> > {
> > @@ -287,6 +308,9 @@ static int raw_recv(struct transport *t, int fd,
> void *buf, int buflen,
> > if (cnt < 0)
> > return cnt;
> >
> > + if (has_prp_trailer(buf, cnt) == 0)
> > + cnt -= PRP_TRAILER_LEN;
> > +
> > if (raw->vlan) {
> > if (ETH_P_1588 == ntohs(hdr->type)) {
> > pr_notice("raw: disabling VLAN mode");
> > --
> > 2.25.1
> >
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
