Thread: [Linuxptp-devel] [PATCH 0/1] Strip PRP trailer
PTP IEEE 1588 stack for Linux
Brought to you by:
rcochran
Menu
▾
▴
linuxptp-devel
|
From: Magnus A. <mag...@gm...> - 2022-06-29 12:29:18
|
In a PRP setup, each outgoing frame is appended with a trailer (RCT) after the normal payload. In order to support PTP over PRP, the trailer needs to be ignored. The following patch has been tested with the software PRP support implemented in the linux kernel 5.9 release. Magnus Armholt (1): Strip PRP trailer raw.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) -- 2.25.1 |
|
From: Magnus A. <mag...@gm...> - 2022-06-29 12:29:21
|
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.
Signed-off-by: Magnus Armholt <mag...@fi...>
---
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_FILTER 12
#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, &addr->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
|
|
From: Miroslav L. <mli...@re...> - 2022-06-29 12:59:49
|
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
|
|
From: Magnus A. <mag...@fi...> - 2022-06-29 17:29:58
|
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
|
|
From: Magnus A. <mag...@fi...> - 2022-06-30 08:15:37
|
> > > + 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). |
|
From: Miroslav L. <mli...@re...> - 2022-06-30 10:32:24
|
On Thu, Jun 30, 2022 at 08:15:26AM +0000, 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 |
|
From: Magnus A. <mag...@gm...> - 2022-07-13 06:30:48
|
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.
Signed-off-by: Magnus Armholt <mag...@fi...>
---
raw.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/raw.c b/raw.c
index ce64684..6c96028 100644
--- a/raw.c
+++ b/raw.c
@@ -65,6 +65,8 @@ struct raw {
#define N_RAW_FILTER 12
#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,39 @@ static void addr_to_mac(void *mac, struct address *addr)
memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
}
+static int has_prp_trailer(unsigned char *ptr, int cnt)
+{
+ unsigned short suffix_id, lane_size_field, lsdu_size;
+
+ if (cnt < PRP_TRAILER_LEN)
+ return -1;
+
+ /* PRP trailer (RCT) consists of 3 uint16.
+ * --------------------------------------------------------*
+ * SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) *
+ * --------------------------------------------------------*
+ - Sequence number is a running number and can't be verified
+ - LanId should be 0x1010 or 0x1011 (but should not be used for verification)
+ - LSDUsize should match LSDU length
+ (including possible padding and the RCT itself)
+ - Suffix should be 0x88FB
+ */
+
+ /* verify the suffix first as it is the best identifier */
+ suffix_id = ntohs(*(unsigned short*)(ptr + (cnt - 2)));
+ if (suffix_id != ETH_P_PRP)
+ return -1;
+
+ /* verify also that the size in the RCT matches */
+ lane_size_field = ntohs(*(unsigned short*)(ptr + (cnt - 4)));
+ // size is lower 12 bits
+ 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 +322,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
|
|
From: Miroslav L. <mli...@re...> - 2022-07-13 07:02:04
|
On Wed, Jul 13, 2022 at 09:30:19AM +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)
> +{
> + unsigned short suffix_id, lane_size_field, lsdu_size;
> +
> + if (cnt < PRP_TRAILER_LEN)
> + return -1;
I thought the function would also cast ptr to ptp_header and check
that the PTP message length is equal to cnt - PRP_TRAILER_LEN before
looking at the trailer in order to avoid false positives causing
corruption of non-PRP frames.
> + /* verify also that the size in the RCT matches */
> + lane_size_field = ntohs(*(unsigned short*)(ptr + (cnt - 4)));
> + // size is lower 12 bits
This is a C++-style comment.
> + lsdu_size = (lane_size_field & 0x0FFF);
> + if (lsdu_size == cnt)
> + return 0;
> +
> + return -1;
A function which has "has" in its name should return 0 (false) or 1
(true), instead of success (0) or error (-1).
--
Miroslav Lichvar
|
|
From: Magnus A. <mag...@gm...> - 2022-07-14 09:25:00
|
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.
Signed-off-by: Magnus Armholt <mag...@fi...>
---
raw.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/raw.c b/raw.c
index ce64684..f57870f 100644
--- a/raw.c
+++ b/raw.c
@@ -65,6 +65,9 @@ struct raw {
#define N_RAW_FILTER 12
#define RAW_FILTER_TEST 9
+#define PRP_MIN_PACKET_LEN 70
+#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 +209,64 @@ static void addr_to_mac(void *mac, struct address *addr)
memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
}
+static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen, int *trailer_len)
+{
+ unsigned short suffix_id, lane_size_field, lsdu_size;
+ struct ptp_header* hdr;
+ int ptp_msg_len, trailer_start, padding_len;
+
+ /* try to parse like a PTP message to find out the message length */
+ if (cnt < sizeof(struct ptp_header))
+ return 0;
+
+ hdr = (struct ptp_header*)ptr;
+ if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
+ return 0;
+
+ ptp_msg_len = ntohs(hdr->messageLength);
+
+ /* PRP requires ethernet packets to be minimum 70 bytes, including trailer */
+ trailer_start = ptp_msg_len;
+ padding_len = 0;
+ if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) < PRP_MIN_PACKET_LEN)
+ {
+ padding_len = PRP_MIN_PACKET_LEN - (eth_hlen + ptp_msg_len + PRP_TRAILER_LEN);
+ trailer_start += padding_len;
+ }
+
+ if (cnt < (trailer_start + PRP_TRAILER_LEN))
+ return 0;
+
+ /* PRP trailer (RCT) consists of 3 uint16.
+ | -------------------------------------------------------- |
+ | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
+ | -------------------------------------------------------- |
+ - Sequence number is a running number and can't be verified
+ - LanId should be 0x1010 or 0x1011 (but should not be used for verification)
+ - LSDUsize should match LSDU length
+ (including possible padding and the RCT itself)
+ - Suffix should be 0x88FB
+ */
+
+ /* Verify that the size in the RCT matches.
+ Size is the lower 12 bits
+ */
+ lane_size_field = ntohs(*(unsigned short*)(ptr + (trailer_start + 2)));
+ lsdu_size = (lane_size_field & 0x0FFF);
+ if (lsdu_size != cnt)
+ return 0;
+
+ /* Verify the suffix */
+ suffix_id = ntohs(*(unsigned short*)(ptr + (trailer_start + 4)));
+ if (suffix_id == ETH_P_PRP)
+ {
+ *trailer_len = PRP_TRAILER_LEN + padding_len;
+ return 1;
+ }
+
+ return 0;
+}
+
static int raw_open(struct transport *t, struct interface *iface,
struct fdarray *fda, enum timestamp_type ts_type)
{
@@ -266,7 +327,7 @@ no_mac:
static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
struct address *addr, struct hw_timestamp *hwts)
{
- int cnt, hlen;
+ int cnt, hlen, trailer_len;
unsigned char *ptr = buf;
struct eth_hdr *hdr;
struct raw *raw = container_of(t, struct raw, t);
@@ -287,6 +348,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, hlen, &trailer_len))
+ cnt -= trailer_len;
+
if (raw->vlan) {
if (ETH_P_1588 == ntohs(hdr->type)) {
pr_notice("raw: disabling VLAN mode");
--
2.25.1
|
|
From: Miroslav L. <mli...@re...> - 2022-07-14 09:45:17
|
On Thu, Jul 14, 2022 at 12:24:39PM +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, int eth_hlen, int *trailer_len)
> +{
> + unsigned short suffix_id, lane_size_field, lsdu_size;
> + struct ptp_header* hdr;
> + int ptp_msg_len, trailer_start, padding_len;
Just some minor coding style issues. Please order the declarations by
length (reverse christmas tree) and write hdr as "*hdr" instead of
"* hdr".
> + /* try to parse like a PTP message to find out the message length */
> + if (cnt < sizeof(struct ptp_header))
> + return 0;
> +
> + hdr = (struct ptp_header*)ptr;
"ptp_header *".
> + lane_size_field = ntohs(*(unsigned short*)(ptr + (trailer_start + 2)));
Unnecessary parenthesis around "trailer_start + 2".
> + lsdu_size = (lane_size_field & 0x0FFF);
Same here.
> + if (lsdu_size != cnt)
> + return 0;
> +
> + /* Verify the suffix */
> + suffix_id = ntohs(*(unsigned short*)(ptr + (trailer_start + 4)));
And here.
> + if (suffix_id == ETH_P_PRP)
> + {
> + *trailer_len = PRP_TRAILER_LEN + padding_len;
> + return 1;
Is it necessary to return the padding length here? If I understand it
correctly it's not specific to PRP. It's just the original Ethernet
minimum length of 64 plus 6 bytes for the trailer. If you remove the
padding_len variable, the function can be simplified a bit.
--
Miroslav Lichvar
|
|
From: Richard C. <ric...@gm...> - 2022-07-25 01:04:05
|
On Thu, Jul 14, 2022 at 11:45:04AM +0200, Miroslav Lichvar wrote:
> On Thu, Jul 14, 2022 at 12:24:39PM +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, int eth_hlen, int *trailer_len)
> > +{
> > + unsigned short suffix_id, lane_size_field, lsdu_size;
> > + struct ptp_header* hdr;
> > + int ptp_msg_len, trailer_start, padding_len;
>
> Just some minor coding style issues. Please order the declarations by
> length (reverse christmas tree) and write hdr as "*hdr" instead of
> "* hdr".
Magnus, please incorporate this and the following feedback and post a v4.
Thanks,
Richard
>
> > + /* try to parse like a PTP message to find out the message length */
> > + if (cnt < sizeof(struct ptp_header))
> > + return 0;
> > +
> > + hdr = (struct ptp_header*)ptr;
>
> "ptp_header *".
>
> > + lane_size_field = ntohs(*(unsigned short*)(ptr + (trailer_start + 2)));
>
> Unnecessary parenthesis around "trailer_start + 2".
>
> > + lsdu_size = (lane_size_field & 0x0FFF);
>
> Same here.
>
> > + if (lsdu_size != cnt)
> > + return 0;
> > +
> > + /* Verify the suffix */
> > + suffix_id = ntohs(*(unsigned short*)(ptr + (trailer_start + 4)));
>
> And here.
>
> > + if (suffix_id == ETH_P_PRP)
> > + {
> > + *trailer_len = PRP_TRAILER_LEN + padding_len;
> > + return 1;
>
> Is it necessary to return the padding length here? If I understand it
> correctly it's not specific to PRP. It's just the original Ethernet
> minimum length of 64 plus 6 bytes for the trailer. If you remove the
> padding_len variable, the function can be simplified a bit.
>
> --
> Miroslav Lichvar
>
>
>
> _______________________________________________
> Linuxptp-devel mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
|
|
From: Magnus A. <mag...@gm...> - 2022-08-08 09:53:38
|
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.
Signed-off-by: Magnus Armholt <mag...@fi...>
---
raw.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/raw.c b/raw.c
index ce64684..b0278c4 100644
--- a/raw.c
+++ b/raw.c
@@ -65,6 +65,9 @@ struct raw {
#define N_RAW_FILTER 12
#define RAW_FILTER_TEST 9
+#define PRP_MIN_PACKET_LEN 70
+#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 +209,63 @@ static void addr_to_mac(void *mac, struct address *addr)
memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
}
+static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen)
+{
+ unsigned short suffix_id, lane_size_field, lsdu_size;
+ int ptp_msg_len, trailer_start, padding_len;
+ struct ptp_header *hdr;
+
+ /* try to parse like a PTP message to find out the message length */
+ if (cnt < sizeof(struct ptp_header))
+ return 0;
+
+ hdr = (struct ptp_header *)ptr;
+ if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
+ return 0;
+
+ ptp_msg_len = ntohs(hdr->messageLength);
+
+ /* PRP requires ethernet packets to be minimum 70 bytes, including trailer */
+ trailer_start = ptp_msg_len;
+ padding_len = 0;
+ if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) < PRP_MIN_PACKET_LEN)
+ {
+ padding_len = PRP_MIN_PACKET_LEN - (eth_hlen + ptp_msg_len + PRP_TRAILER_LEN);
+ trailer_start += padding_len;
+ }
+
+ if (cnt < (trailer_start + PRP_TRAILER_LEN))
+ return 0;
+
+ /* PRP trailer (RCT) consists of 3 uint16.
+ | -------------------------------------------------------- |
+ | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
+ | -------------------------------------------------------- |
+ - Sequence number is a running number and can't be verified
+ - LanId should be 0x1010 or 0x1011 (but should not be used for verification)
+ - LSDUsize should match LSDU length
+ (including possible padding and the RCT itself)
+ - Suffix should be 0x88FB
+ */
+
+ /* Verify that the size in the RCT matches.
+ Size is the lower 12 bits
+ */
+ lane_size_field = ntohs(*(unsigned short*)(ptr + trailer_start + 2));
+ lsdu_size = lane_size_field & 0x0FFF;
+ if (lsdu_size != cnt)
+ return 0;
+
+ /* Verify the suffix */
+ suffix_id = ntohs(*(unsigned short*)(ptr + trailer_start + 4));
+ if (suffix_id == ETH_P_PRP)
+ {
+ return 1;
+ }
+
+ return 0;
+}
+
static int raw_open(struct transport *t, struct interface *iface,
struct fdarray *fda, enum timestamp_type ts_type)
{
@@ -266,10 +326,10 @@ no_mac:
static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
struct address *addr, struct hw_timestamp *hwts)
{
- int cnt, hlen;
+ struct raw *raw = container_of(t, struct raw, t);
unsigned char *ptr = buf;
struct eth_hdr *hdr;
- struct raw *raw = container_of(t, struct raw, t);
+ int cnt, hlen;
if (raw->vlan) {
hlen = sizeof(struct vlan_hdr);
@@ -287,6 +347,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, hlen))
+ cnt -= PRP_TRAILER_LEN;
+
if (raw->vlan) {
if (ETH_P_1588 == ntohs(hdr->type)) {
pr_notice("raw: disabling VLAN mode");
--
2.25.1
|
|
From: Erez <ere...@gm...> - 2022-08-08 10:21:15
|
On Mon, 8 Aug 2022 at 11:56, Magnus Armholt <mag...@gm...>
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.
>
>
Perhaps, it is possible to add this function under a compilation flag?
As most users do not plan to use PRP.
Richard, what do you think of compilation flags?
> Signed-off-by: Magnus Armholt <mag...@fi...>
> ---
> raw.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/raw.c b/raw.c
> index ce64684..b0278c4 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -65,6 +65,9 @@ struct raw {
> #define N_RAW_FILTER 12
> #define RAW_FILTER_TEST 9
>
> +#define PRP_MIN_PACKET_LEN 70
> +#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 +209,63 @@ static void addr_to_mac(void *mac, struct address
> *addr)
> memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
> }
>
>
Although the function is local, it would be nice to give a short
description with
the name of the protocol, this function probe.
And no need to repeat the PRP name, use a protocol name people can
understand.
> +static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen)
>
A POSIX function usually uses 0 as success and negative on failure.
In order to avoid confusion, it is better to use the boolean type here.
You may include <stdbool.h> to define the type.
+{
> + unsigned short suffix_id, lane_size_field, lsdu_size;
> + int ptp_msg_len, trailer_start, padding_len;
> + struct ptp_header *hdr;
> +
> + /* try to parse like a PTP message to find out the message length
> */
> + if (cnt < sizeof(struct ptp_header))
> + return 0;
>
Please use false here.
> +
> + hdr = (struct ptp_header *)ptr;
> + if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
> + return 0;
>
Please use false here.
> +
> + ptp_msg_len = ntohs(hdr->messageLength);
> +
> + /* PRP requires ethernet packets to be minimum 70 bytes, including
> trailer */
> + trailer_start = ptp_msg_len;
> + padding_len = 0;
> + if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) <
> PRP_MIN_PACKET_LEN)
> + {
> + padding_len = PRP_MIN_PACKET_LEN - (eth_hlen + ptp_msg_len
> + PRP_TRAILER_LEN);
> + trailer_start += padding_len;
> + }
> +
> + if (cnt < (trailer_start + PRP_TRAILER_LEN))
> + return 0;
>
Please use false here.
+
> + /* PRP trailer (RCT) consists of 3 uint16.
> + | -------------------------------------------------------- |
> + | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
> + | -------------------------------------------------------- |
> + - Sequence number is a running number and can't be verified
> + - LanId should be 0x1010 or 0x1011 (but should not be used for
> verification)
> + - LSDUsize should match LSDU length
> + (including possible padding and the RCT itself)
> + - Suffix should be 0x88FB
> + */
> +
> + /* Verify that the size in the RCT matches.
> + Size is the lower 12 bits
> + */
> + lane_size_field = ntohs(*(unsigned short*)(ptr + trailer_start +
> 2));
> + lsdu_size = lane_size_field & 0x0FFF;
> + if (lsdu_size != cnt)
> + return 0;
>
Please use false here.
> +
> + /* Verify the suffix */
> + suffix_id = ntohs(*(unsigned short*)(ptr + trailer_start + 4));
> + if (suffix_id == ETH_P_PRP)
> + {
> + return 1;
>
Please use true here.
> + }
> +
> + return 0;
>
Please use false here.
> +}
> +
> static int raw_open(struct transport *t, struct interface *iface,
> struct fdarray *fda, enum timestamp_type ts_type)
> {
> @@ -266,10 +326,10 @@ no_mac:
> static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
> struct address *addr, struct hw_timestamp *hwts)
> {
> - int cnt, hlen;
> + struct raw *raw = container_of(t, struct raw, t);
> unsigned char *ptr = buf;
> struct eth_hdr *hdr;
> - struct raw *raw = container_of(t, struct raw, t);
> + int cnt, hlen;
>
> if (raw->vlan) {
> hlen = sizeof(struct vlan_hdr);
> @@ -287,6 +347,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, hlen))
>
This makes sense if has_prp_trailer() returns boolean.
+ 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
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
>
|
|
From: Miroslav L. <mli...@re...> - 2022-08-08 10:36:46
|
On Mon, Aug 08, 2022 at 12:20:30PM +0200, Erez wrote: > On Mon, 8 Aug 2022 at 11:56, Magnus Armholt <mag...@gm...> > wrote: > > +static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen) > > > > A POSIX function usually uses 0 as success and negative on failure. > In order to avoid confusion, it is better to use the boolean type here. > You may include <stdbool.h> to define the type. There are other is_* and has_* functions that return 1 for true. It's not a success or failure. If they should be declared as boolean, it would be nice to convert them all avoid confusion. -- Miroslav Lichvar |
|
From: Erez <ere...@gm...> - 2022-08-08 11:12:20
|
On Mon, 8 Aug 2022 at 12:36, Miroslav Lichvar <mli...@re...> wrote: > On Mon, Aug 08, 2022 at 12:20:30PM +0200, Erez wrote: > > On Mon, 8 Aug 2022 at 11:56, Magnus Armholt <mag...@gm...> > > wrote: > > > +static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen) > > > > > > > A POSIX function usually uses 0 as success and negative on failure. > > In order to avoid confusion, it is better to use the boolean type here. > > You may include <stdbool.h> to define the type. > > There are other is_* and has_* functions that return 1 for true. It's > not a success or failure. If they should be declared as boolean, it > bool is used. For example in "interface.h" bool interface_tsinfo_valid(struct interface *iface); "_valid" does suggest boolean, though might be ambiguous. "is_" is the best choice for boolean value. > would be nice to convert them all avoid confusion. > Richard usually does not require rebase of all code to a single convention. For example, new code uses the SPDX tags, while old code does not. The question is not if other code uses it, but what is better. Though the function name suggests boolean. I think we should do both, to make code clearer and more readable. It also makes the patch more coherent. Erez > -- > Miroslav Lichvar > > |
|
From: Richard C. <ric...@gm...> - 2022-08-08 13:59:35
|
On Mon, Aug 08, 2022 at 01:11:36PM +0200, Erez wrote: > On Mon, 8 Aug 2022 at 12:36, Miroslav Lichvar <mli...@re...> wrote: > > would be nice to convert them all avoid confusion. Yes, but not here with new code. > The question is not if other code uses it, but what is better. > Though the function name suggests boolean. > I think we should do both, to make code clearer and more readable. > It also makes the patch more coherent. Yeah, please have new code use Boolean type when functions are predicates (is_a, has_a, etc). (I used to avoid bool because of poor compilers back in the Bad Old Days) Thanks, Richard |
|
From: Magnus A. <mag...@gm...> - 2022-08-09 05:26:37
|
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.
The PRP trailer is normally removed by the PRP implementation
and invisible to the rest of the stack.
If a PRP HW implementation is used the trailer won't be there.
When the SW PRP implementataion introduction in linux kernel 5.9
is used, the trailer will 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 it is needed to run
a ptp4l instance on each interface instead of on the combined
prp0 interface created by the kernel.
Hence, the PRP trailer will still be there in PTP packages
received directly from the interfaces, the PRP stack hasn't
had the chance to remove it
(which it would if traffic is read from prp0).
Signed-off-by: Magnus Armholt <mag...@fi...>
---
raw.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 2 deletions(-)
diff --git a/raw.c b/raw.c
index ce64684..7fa2287 100644
--- a/raw.c
+++ b/raw.c
@@ -23,6 +23,7 @@
#include <net/if.h>
#include <netinet/in.h>
#include <netpacket/packet.h>
+#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -65,6 +66,9 @@ struct raw {
#define N_RAW_FILTER 12
#define RAW_FILTER_TEST 9
+#define PRP_MIN_PACKET_LEN 70
+#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 +210,64 @@ static void addr_to_mac(void *mac, struct address *addr)
memcpy(mac, &addr->sll.sll_addr, MAC_LEN);
}
+/* Determines if the packet has Parallel Redundancy Protocol (PRP) trailer. */
+static bool has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen)
+{
+ unsigned short suffix_id, lane_size_field, lsdu_size;
+ int ptp_msg_len, trailer_start, padding_len;
+ struct ptp_header *hdr;
+
+ /* try to parse like a PTP message to find out the message length */
+ if (cnt < sizeof(struct ptp_header))
+ return false;
+
+ hdr = (struct ptp_header *)ptr;
+ if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
+ return false;
+
+ ptp_msg_len = ntohs(hdr->messageLength);
+
+ /* PRP requires ethernet packets to be minimum 70 bytes, including trailer */
+ trailer_start = ptp_msg_len;
+ padding_len = 0;
+ if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) < PRP_MIN_PACKET_LEN)
+ {
+ padding_len = PRP_MIN_PACKET_LEN - (eth_hlen + ptp_msg_len + PRP_TRAILER_LEN);
+ trailer_start += padding_len;
+ }
+
+ if (cnt < (trailer_start + PRP_TRAILER_LEN))
+ return false;
+
+ /* PRP trailer (RCT) consists of 3 uint16.
+ | -------------------------------------------------------- |
+ | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
+ | -------------------------------------------------------- |
+ - Sequence number is a running number and can't be verified
+ - LanId should be 0x1010 or 0x1011 (but should not be used for verification)
+ - LSDUsize should match LSDU length
+ (including possible padding and the RCT itself)
+ - Suffix should be 0x88FB
+ */
+
+ /* Verify that the size in the RCT matches.
+ Size is the lower 12 bits
+ */
+ lane_size_field = ntohs(*(unsigned short*)(ptr + trailer_start + 2));
+ lsdu_size = lane_size_field & 0x0FFF;
+ if (lsdu_size != cnt)
+ return false;
+
+ /* Verify the suffix */
+ suffix_id = ntohs(*(unsigned short*)(ptr + trailer_start + 4));
+ if (suffix_id == ETH_P_PRP)
+ {
+ return true;
+ }
+
+ return false;
+}
+
static int raw_open(struct transport *t, struct interface *iface,
struct fdarray *fda, enum timestamp_type ts_type)
{
@@ -266,10 +328,10 @@ no_mac:
static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
struct address *addr, struct hw_timestamp *hwts)
{
- int cnt, hlen;
+ struct raw *raw = container_of(t, struct raw, t);
unsigned char *ptr = buf;
struct eth_hdr *hdr;
- struct raw *raw = container_of(t, struct raw, t);
+ int cnt, hlen;
if (raw->vlan) {
hlen = sizeof(struct vlan_hdr);
@@ -287,6 +349,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, hlen))
+ cnt -= PRP_TRAILER_LEN;
+
if (raw->vlan) {
if (ETH_P_1588 == ntohs(hdr->type)) {
pr_notice("raw: disabling VLAN mode");
--
2.25.1
|
|
From: Richard C. <ric...@gm...> - 2022-10-08 20:44:31
|
On Tue, Aug 09, 2022 at 08:26:08AM +0300, Magnus Armholt wrote:
> Strip the IEC62439-3 PRP trailer if it is present
> to support PTP over PRP.
Fails when building against Linux kernel 3.0 headers (the earliest
supported kernel version).
/home/richard/git/linuxptp/raw.c: In function ‘has_prp_trailer’:
/home/richard/git/linuxptp/raw.c:263:19: error: ‘ETH_P_PRP’ undeclared (first use in this function); did you mean ‘ETH_P_ARP’?
263 | if (suffix_id == ETH_P_PRP)
| ^~~~~~~~~
| ETH_P_ARP
Please fix that, and then this is good to go.
Thanks,
Richard
|
|
From: Magnus A. <mag...@fi...> - 2022-08-30 20:58:00
|
Hi,
Any more review comments related to this patch or is there a chance it could be accepted?
BR,
Magnus Armholt
> -----Original Message-----
> From: Magnus Armholt <mag...@gm...>
> Sent: tiistai 9. elokuuta 2022 8.26
> To: lin...@li...
> Cc: Magnus Armholt <mag...@fi...>
> Subject: [PATCH v5] Strip Parallel Redundancy Protocol (PRP) trailer
>
> BeSecure! This email comes from outside of ABB. Make sure you verify
> the sender before clicking any links or downloading/opening attachments.
> If this email looks suspicious, report it by clicking 'Report Phishing' button in
> Outlook or raising a ticket on MyIS.
>
>
> 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.
>
> The PRP trailer is normally removed by the PRP implementation and invisible
> to the rest of the stack.
> If a PRP HW implementation is used the trailer won't be there.
> When the SW PRP implementataion introduction in linux kernel 5.9 is used,
> the trailer will 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 it is needed to run a ptp4l instance on each interface instead of
> on the combined
> prp0 interface created by the kernel.
> Hence, the PRP trailer will still be there in PTP packages received directly
> from the interfaces, the PRP stack hasn't had the chance to remove it (which
> it would if traffic is read from prp0).
>
> Signed-off-by: Magnus Armholt <mag...@fi...>
> ---
> raw.c | 69
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/raw.c b/raw.c
> index ce64684..7fa2287 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -23,6 +23,7 @@
> #include <net/if.h>
> #include <netinet/in.h>
> #include <netpacket/packet.h>
> +#include <stdbool.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -65,6 +66,9 @@ struct raw {
> #define N_RAW_FILTER 12
> #define RAW_FILTER_TEST 9
>
> +#define PRP_MIN_PACKET_LEN 70
> +#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 +210,64 @@ static void addr_to_mac(void *mac, struct address
> *addr)
> memcpy(mac, &addr->sll.sll_addr, MAC_LEN); }
>
> +/* Determines if the packet has Parallel Redundancy Protocol (PRP)
> +trailer. */ static bool has_prp_trailer(unsigned char *ptr, int cnt,
> +int eth_hlen) {
> + unsigned short suffix_id, lane_size_field, lsdu_size;
> + int ptp_msg_len, trailer_start, padding_len;
> + struct ptp_header *hdr;
> +
> + /* try to parse like a PTP message to find out the message length */
> + if (cnt < sizeof(struct ptp_header))
> + return false;
> +
> + hdr = (struct ptp_header *)ptr;
> + if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
> + return false;
> +
> + ptp_msg_len = ntohs(hdr->messageLength);
> +
> + /* PRP requires ethernet packets to be minimum 70 bytes, including
> trailer */
> + trailer_start = ptp_msg_len;
> + padding_len = 0;
> + if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) <
> PRP_MIN_PACKET_LEN)
> + {
> + padding_len = PRP_MIN_PACKET_LEN - (eth_hlen + ptp_msg_len +
> PRP_TRAILER_LEN);
> + trailer_start += padding_len;
> + }
> +
> + if (cnt < (trailer_start + PRP_TRAILER_LEN))
> + return false;
> +
> + /* PRP trailer (RCT) consists of 3 uint16.
> + | -------------------------------------------------------- |
> + | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
> + | -------------------------------------------------------- |
> + - Sequence number is a running number and can't be verified
> + - LanId should be 0x1010 or 0x1011 (but should not be used for
> verification)
> + - LSDUsize should match LSDU length
> + (including possible padding and the RCT itself)
> + - Suffix should be 0x88FB
> + */
> +
> + /* Verify that the size in the RCT matches.
> + Size is the lower 12 bits
> + */
> + lane_size_field = ntohs(*(unsigned short*)(ptr + trailer_start + 2));
> + lsdu_size = lane_size_field & 0x0FFF;
> + if (lsdu_size != cnt)
> + return false;
> +
> + /* Verify the suffix */
> + suffix_id = ntohs(*(unsigned short*)(ptr + trailer_start + 4));
> + if (suffix_id == ETH_P_PRP)
> + {
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int raw_open(struct transport *t, struct interface *iface,
> struct fdarray *fda, enum timestamp_type ts_type) { @@ -266,10
> +328,10 @@ no_mac:
> static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
> struct address *addr, struct hw_timestamp *hwts) {
> - int cnt, hlen;
> + struct raw *raw = container_of(t, struct raw, t);
> unsigned char *ptr = buf;
> struct eth_hdr *hdr;
> - struct raw *raw = container_of(t, struct raw, t);
> + int cnt, hlen;
>
> if (raw->vlan) {
> hlen = sizeof(struct vlan_hdr); @@ -287,6 +349,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, hlen))
> + cnt -= PRP_TRAILER_LEN;
> +
> if (raw->vlan) {
> if (ETH_P_1588 == ntohs(hdr->type)) {
> pr_notice("raw: disabling VLAN mode");
> --
> 2.25.1
|
|
From: Magnus A. <mag...@fi...> - 2022-09-19 12:14:55
|
Hi,
Sorry to keep polling about this.
Is there something more I can do for this patch, or is it just to wait?
BR,
Magnus Armholt
> -----Original Message-----
> From: Magnus Armholt via Linuxptp-devel <linuxptp-
> de...@li...>
> Sent: tiistai 30. elokuuta 2022 21.25
> To: lin...@li...
> Subject: Re: [Linuxptp-devel] [PATCH v5] Strip Parallel Redundancy Protocol
> (PRP) trailer
>
> Hi,
>
> Any more review comments related to this patch or is there a chance it could
> be accepted?
>
> BR,
> Magnus Armholt
>
> > -----Original Message-----
> > From: Magnus Armholt <mag...@gm...>
> > Sent: tiistai 9. elokuuta 2022 8.26
> > To: lin...@li...
> > Cc: Magnus Armholt <mag...@fi...>
> > Subject: [PATCH v5] Strip Parallel Redundancy Protocol (PRP) trailer
> >
> > 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.
> >
> > The PRP trailer is normally removed by the PRP implementation and
> > invisible to the rest of the stack.
> > If a PRP HW implementation is used the trailer won't be there.
> > When the SW PRP implementataion introduction in linux kernel 5.9 is
> > used, the trailer will 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 it is needed to run a ptp4l instance on each interface
> > instead of on the combined
> > prp0 interface created by the kernel.
> > Hence, the PRP trailer will still be there in PTP packages received
> > directly from the interfaces, the PRP stack hasn't had the chance to
> > remove it (which it would if traffic is read from prp0).
> >
> > Signed-off-by: Magnus Armholt <mag...@fi...>
> > ---
> > raw.c | 69
> >
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/raw.c b/raw.c
> > index ce64684..7fa2287 100644
> > --- a/raw.c
> > +++ b/raw.c
> > @@ -23,6 +23,7 @@
> > #include <net/if.h>
> > #include <netinet/in.h>
> > #include <netpacket/packet.h>
> > +#include <stdbool.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > @@ -65,6 +66,9 @@ struct raw {
> > #define N_RAW_FILTER 12
> > #define RAW_FILTER_TEST 9
> >
> > +#define PRP_MIN_PACKET_LEN 70
> > +#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 +210,64 @@ static void addr_to_mac(void *mac, struct address
> > *addr)
> > memcpy(mac, &addr->sll.sll_addr, MAC_LEN); }
> >
> > +/* Determines if the packet has Parallel Redundancy Protocol (PRP)
> > +trailer. */ static bool has_prp_trailer(unsigned char *ptr, int cnt,
> > +int eth_hlen) {
> > + unsigned short suffix_id, lane_size_field, lsdu_size;
> > + int ptp_msg_len, trailer_start, padding_len;
> > + struct ptp_header *hdr;
> > +
> > + /* try to parse like a PTP message to find out the message length */
> > + if (cnt < sizeof(struct ptp_header))
> > + return false;
> > +
> > + hdr = (struct ptp_header *)ptr;
> > + if ((hdr->ver & MAJOR_VERSION_MASK) != PTP_MAJOR_VERSION)
> > + return false;
> > +
> > + ptp_msg_len = ntohs(hdr->messageLength);
> > +
> > + /* PRP requires ethernet packets to be minimum 70 bytes,
> > + including
> > trailer */
> > + trailer_start = ptp_msg_len;
> > + padding_len = 0;
> > + if ((eth_hlen + ptp_msg_len + PRP_TRAILER_LEN) <
> > PRP_MIN_PACKET_LEN)
> > + {
> > + padding_len = PRP_MIN_PACKET_LEN - (eth_hlen +
> > + ptp_msg_len +
> > PRP_TRAILER_LEN);
> > + trailer_start += padding_len;
> > + }
> > +
> > + if (cnt < (trailer_start + PRP_TRAILER_LEN))
> > + return false;
> > +
> > + /* PRP trailer (RCT) consists of 3 uint16.
> > + | -------------------------------------------------------- |
> > + | SeqNr(0-15) | LanId(0-3) LSDUsize(4-15) | Suffix (0-15) |
> > + | -------------------------------------------------------- |
> > + - Sequence number is a running number and can't be verified
> > + - LanId should be 0x1010 or 0x1011 (but should not be used
> > + for
> > verification)
> > + - LSDUsize should match LSDU length
> > + (including possible padding and the RCT itself)
> > + - Suffix should be 0x88FB
> > + */
> > +
> > + /* Verify that the size in the RCT matches.
> > + Size is the lower 12 bits
> > + */
> > + lane_size_field = ntohs(*(unsigned short*)(ptr + trailer_start + 2));
> > + lsdu_size = lane_size_field & 0x0FFF;
> > + if (lsdu_size != cnt)
> > + return false;
> > +
> > + /* Verify the suffix */
> > + suffix_id = ntohs(*(unsigned short*)(ptr + trailer_start + 4));
> > + if (suffix_id == ETH_P_PRP)
> > + {
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > static int raw_open(struct transport *t, struct interface *iface,
> > struct fdarray *fda, enum timestamp_type ts_type)
> > { @@ -266,10
> > +328,10 @@ no_mac:
> > static int raw_recv(struct transport *t, int fd, void *buf, int buflen,
> > struct address *addr, struct hw_timestamp *hwts) {
> > - int cnt, hlen;
> > + struct raw *raw = container_of(t, struct raw, t);
> > unsigned char *ptr = buf;
> > struct eth_hdr *hdr;
> > - struct raw *raw = container_of(t, struct raw, t);
> > + int cnt, hlen;
> >
> > if (raw->vlan) {
> > hlen = sizeof(struct vlan_hdr); @@ -287,6 +349,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, hlen))
> > + 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
> Lin...@li...
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.s
> ourceforge.net%2Flists%2Flistinfo%2Flinuxptp-
> devel&data=05%7C01%7Cmagnus.armholt%40fi.abb.com%7C8ef07cd93
> 05542d288c308da8aca8a30%7C372ee9e09ce04033a64ac07073a91ecd%7C0%7
> C0%7C637974899754348290%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&sdata=0ct8T0NCDdLv89HvFS3sOJ0b7wN%2FlvBH6N8qKDDqm
> zk%3D&reserved=0
|
|
From: Magnus A. <mag...@fi...> - 2022-07-15 06:06:23
|
Hi, > Just some minor coding style issues. Please order the declarations by length > (reverse christmas tree) and write hdr as "*hdr" instead of > "* hdr". Thanks for pointing out the coding style, will fix it. > > Is it necessary to return the padding length here? If I understand it correctly > it's not specific to PRP. It's just the original Ethernet minimum length of 64 > plus 6 bytes for the trailer. If you remove the padding_len variable, the > function can be simplified a bit. > In my testing I haven't noticed any issues if I only remove the actual PRP trailer (and not padding), but it might because the padding is 00 bytes. For example, a PTP SYNC message is 44bytes long, with normal eth header (no vlan) being 14bytes it ends up being padded with 6 bytes (followed by PRP trailer). If I only remove the PRP trailer (6 bytes) but leave the padding length in `cnt`, then if I follow the code correctly the message should end up in msg.c:425 and still have 6 (padding)bytes to process. To my understand it should try to parse TLV, and then return a suffix_len of 4 (successfully taken 1 TLV and failed to take the 2nd) which should fail the check on msg.c:429. This is just from reading the code, might be that I am missing something. BR, Magnus Armholt |
|
From: Magnus A. <mag...@fi...> - 2022-07-15 06:49:31
|
> > Is it necessary to return the padding length here? If I understand it > > correctly it's not specific to PRP. It's just the original Ethernet > > minimum length of 64 plus 6 bytes for the trailer. If you remove the > > padding_len variable, the function can be simplified a bit. > > > In my testing I haven't noticed any issues if I only remove the actual PRP > trailer (and not padding), but it might because the padding is 00 bytes. > > For example, a PTP SYNC message is 44bytes long, with normal eth header > (no vlan) being 14bytes it ends up being padded with 6 bytes (followed by > PRP trailer). > If I only remove the PRP trailer (6 bytes) but leave the padding length in `cnt`, > then if I follow the code correctly the message should end up in msg.c:425 > and still have 6 (padding)bytes to process. > To my understand it should try to parse TLV, and then return a suffix_len of 4 > (successfully taken 1 TLV and failed to take the 2nd) which should fail the > check on msg.c:429. > This is just from reading the code, might be that I am missing something. > A packet capture shows that the SYNC message is actually only padded by 2 bytes (I forgot the FCS at end of frame) so that will leave the remaining size (of the padding) to be parsed on msg.c:425 to be smaller than the TLV size (4) so no TLV will be parsed, but the code should still fail the check on msg.c:429. I am obviously missing something still, because I don't see any issues(no bad message indicated, works fine) with the SYNC message, even if I don't remove padding length from `cnt`. - Magnus Armholt |
|
From: Miroslav L. <mli...@re...> - 2022-07-18 07:14:09
|
On Fri, Jul 15, 2022 at 06:16:21AM +0000, Magnus Armholt wrote: > A packet capture shows that the SYNC message is actually only padded by 2 bytes (I forgot the FCS at end of frame) > so that will leave the remaining size (of the padding) to be parsed on msg.c:425 to be smaller than the TLV size (4) > so no TLV will be parsed, but the code should still fail the check on msg.c:429. It shouldn't fail with a 2-byte padding. No TLVs are found, so suffix_len is zero and pdulen + suffix_len should be equal to msg->header.messageLength. -- Miroslav Lichvar |
|
From: Magnus A. <mag...@fi...> - 2022-06-30 14:14:35
|
> > > > + 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 |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X