[Linuxptp-devel] [RFC PATCH] Distinguish between ignored and malformed packets

[Libor Pechacek]

Hello,

I've played with linuxptp in a network where was a host talking PTPv1. All the
announcements the other host sent out were reported as "bad message" by ptp4l
which was pretty annoying.

The question is how to deal with this situation. msg_post_recv() returns zero
if all is OK, and -1 for any other condition. We need to distinguish between
ignored and malformed packets here, possibly with more fine-grained diagnostics
of the error for better field debugging. One way would be to define our own
set of error conditions, another is to re-use the standard set of error numbers
and give them new meaning in the PTP context.

I'm in favor of trying the second approach as drafted below. Opinions?

Libor
---
msg.c | 19 ++++++++++---------
pmc_common.c | 10 +++++++---
port.c | 9 ++++++---
3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/msg.c b/msg.c
index eefd359..5c3f9ac 100644
--- a/msg.c
+++ b/msg.c
@@ -17,6 +17,7 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <arpa/inet.h>
+#include <errno.h>
#include <malloc.h>
#include <string.h>
#include <time.h>
@@ -90,7 +91,7 @@ int64_t net2host64(int64_t val)
static int hdr_post_recv(struct ptp_header *m)
{
if ((m->ver & VERSION_MASK) != VERSION)
- return -1;
+ return -EPROTO;
m->messageLength = ntohs(m->messageLength);
m->correction = net2host64(m->correction);
m->sourcePortIdentity.portNumber = ntohs(m->sourcePortIdentity.portNumber);
@@ -251,14 +252,14 @@ void msg_get(struct ptp_message *m)

int msg_post_recv(struct ptp_message *m, int cnt)
{
- int pdulen, type;
+ int pdulen, type, ret;
uint8_t *suffix = NULL;

if (cnt < sizeof(struct ptp_header))
- return -1;
+ return -EBADMSG;

- if (hdr_post_recv(&m->header))
- return -1;
+ if (ret = hdr_post_recv(&m->header))
+ return ret;

type = msg_type(m);

@@ -294,11 +295,11 @@ int msg_post_recv(struct ptp_message *m, int cnt)
pdulen = sizeof(struct management_msg);
break;
default:
- return -1;
+ return -EBADMSG;
}

if (cnt < pdulen)
- return -1;
+ return -EBADMSG;

switch (type) {
case SYNC:
@@ -341,12 +342,12 @@ int msg_post_recv(struct ptp_message *m, int cnt)

if (msg_sots_missing(m)) {
pr_err("received %s without timestamp", msg_type_string(type));
- return -1;
+ return -EBADMSG;
}

m->tlv_count = suffix_post_recv(suffix, cnt - pdulen, &m->last_tlv);
if (m->tlv_count == -1) {
- return -1;
+ return -EBADMSG;
}

return 0;
diff --git a/pmc_common.c b/pmc_common.c
index ee88bda..cefd038 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -17,6 +17,7 @@
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
+#include <errno.h>
#include <string.h>
#include <stdlib.h>

@@ -163,7 +164,7 @@ int pmc_send_get_action(struct pmc *pmc, int id)
struct ptp_message *pmc_recv(struct pmc *pmc)
{
struct ptp_message *msg;
- int cnt;
+ int cnt, ret;

msg = msg_allocate();
if (!msg) {
@@ -176,8 +177,11 @@ struct ptp_message *pmc_recv(struct pmc *pmc)
if (cnt <= 0) {
pr_err("recv message failed");
goto failed;
- } else if (msg_post_recv(msg, cnt)) {
- pr_err("bad message");
+ } else if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("bad message");
+ else
+ pr_debug("ignoring message");
goto failed;
}

diff --git a/port.c b/port.c
index a1b17b7..141b8ac 100644
--- a/port.c
+++ b/port.c
@@ -1768,7 +1768,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
{
enum fsm_event event = EV_NONE;
struct ptp_message *msg;
- int cnt, fd = p->fda.fd[fd_index];
+ int cnt, fd = p->fda.fd[fd_index], ret;

switch (fd_index) {
case FD_ANNOUNCE_TIMER:
@@ -1813,8 +1813,11 @@ enum fsm_event port_event(struct port *p, int fd_index)
msg_put(msg);
return EV_FAULT_DETECTED;
}
- if (msg_post_recv(msg, cnt)) {
- pr_err("port %hu: bad message", portnum(p));
+ if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("port %hu: bad message", portnum(p));
+ else
+ pr_debug("port %hu: ignoring message", portnum(p));
msg_put(msg);
return EV_NONE;
}

[Richard Cochran]

Post by Libor Pechacek
Hello,
I've played with linuxptp in a network where was a host talking PTPv1. All the
announcements the other host sent out were reported as "bad message" by ptp4l
which was pretty annoying.

Yep, pretty annoying.

Post by Libor Pechacek
The question is how to deal with this situation. msg_post_recv() returns zero
if all is OK, and -1 for any other condition. We need to distinguish between
ignored and malformed packets here, possibly with more fine-grained diagnostics
of the error for better field debugging. One way would be to define our own
set of error conditions, another is to re-use the standard set of error numbers
and give them new meaning in the PTP context.
I'm in favor of trying the second approach as drafted below. Opinions?

I think this way makes sense, and I have a few comment, below.

Post by Libor Pechacek
Libor
---
msg.c | 19 ++++++++++---------
pmc_common.c | 10 +++++++---
port.c | 9 ++++++---
3 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/msg.c b/msg.c
index eefd359..5c3f9ac 100644
--- a/msg.c
+++ b/msg.c
@@ -17,6 +17,7 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <arpa/inet.h>
+#include <errno.h>
#include <malloc.h>
#include <string.h>
#include <time.h>
@@ -90,7 +91,7 @@ int64_t net2host64(int64_t val)
static int hdr_post_recv(struct ptp_header *m)
{
if ((m->ver & VERSION_MASK) != VERSION)
- return -1;
+ return -EPROTO;
m->messageLength = ntohs(m->messageLength);
m->correction = net2host64(m->correction);
m->sourcePortIdentity.portNumber = ntohs(m->sourcePortIdentity.portNumber);
@@ -251,14 +252,14 @@ void msg_get(struct ptp_message *m)
int msg_post_recv(struct ptp_message *m, int cnt)
{
- int pdulen, type;
+ int pdulen, type, ret;

I prefer using 'err' instead of 'ret' for this kind of thing.

Post by Libor Pechacek
uint8_t *suffix = NULL;
if (cnt < sizeof(struct ptp_header))
- return -1;
+ return -EBADMSG;
- if (hdr_post_recv(&m->header))
- return -1;
+ if (ret = hdr_post_recv(&m->header))
+ return ret;

This pattern triggers the 'suggest parentheses' gcc warning, but
adding parenthesis looks bad, IMHO. Instead I suggest doing this:

err = hdr_post_recv(&m->header);
if (err)
return err;

Even though it adds an extra line, to me it is still more legible than

if ((ret = hdr_post_recv(&m->header)) != 0)
return ret;

Post by Libor Pechacek
type = msg_type(m);
@@ -294,11 +295,11 @@ int msg_post_recv(struct ptp_message *m, int cnt)
pdulen = sizeof(struct management_msg);
break;
- return -1;
+ return -EBADMSG;
}
if (cnt < pdulen)
- return -1;
+ return -EBADMSG;
switch (type) {
@@ -341,12 +342,12 @@ int msg_post_recv(struct ptp_message *m, int cnt)
if (msg_sots_missing(m)) {
pr_err("received %s without timestamp", msg_type_string(type));
- return -1;
+ return -EBADMSG;

Maybe this case deserves its own code? Then we could move the error
message into port.c, where it could also show the port ID.

Post by Libor Pechacek
}
m->tlv_count = suffix_post_recv(suffix, cnt - pdulen, &m->last_tlv);
if (m->tlv_count == -1) {
- return -1;
+ return -EBADMSG;

Here we should also propagate the error codes into suffix_post_recv().

Post by Libor Pechacek
}
return 0;
diff --git a/pmc_common.c b/pmc_common.c
index ee88bda..cefd038 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -17,6 +17,7 @@
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
+#include <errno.h>
#include <string.h>
#include <stdlib.h>
@@ -163,7 +164,7 @@ int pmc_send_get_action(struct pmc *pmc, int id)
struct ptp_message *pmc_recv(struct pmc *pmc)
{
struct ptp_message *msg;
- int cnt;
+ int cnt, ret;
msg = msg_allocate();
if (!msg) {
@@ -176,8 +177,11 @@ struct ptp_message *pmc_recv(struct pmc *pmc)
if (cnt <= 0) {
pr_err("recv message failed");
goto failed;
- } else if (msg_post_recv(msg, cnt)) {
- pr_err("bad message");
+ } else if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("bad message");
+ else
+ pr_debug("ignoring message");

Here I suggest:

err = msg_post_recv(msg, cnt);
switch (err) {
...
}

Post by Libor Pechacek
goto failed;
}
diff --git a/port.c b/port.c
index a1b17b7..141b8ac 100644
--- a/port.c
+++ b/port.c
@@ -1768,7 +1768,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
{
enum fsm_event event = EV_NONE;
struct ptp_message *msg;
- int cnt, fd = p->fda.fd[fd_index];
+ int cnt, fd = p->fda.fd[fd_index], ret;
switch (fd_index) {
@@ -1813,8 +1813,11 @@ enum fsm_event port_event(struct port *p, int fd_index)
msg_put(msg);
return EV_FAULT_DETECTED;
}
- if (msg_post_recv(msg, cnt)) {
- pr_err("port %hu: bad message", portnum(p));
+ if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("port %hu: bad message", portnum(p));
+ else
+ pr_debug("port %hu: ignoring message", portnum(p));

Same here, switch/case.

Post by Libor Pechacek
msg_put(msg);
return EV_NONE;
}

Thanks,
Richard

[Libor Pechacek]

[...]

Post by Richard Cochran

One way would be to define our own set of error conditions, another is to
re-use the standard set of error numbers and give them new meaning in the
PTP context.
I'm in favor of trying the second approach as drafted below. Opinions?

I think this way makes sense, and I have a few comment, below.

OK, I'll look into it.

Post by Richard Cochran

Libor
---

[...]

Post by Richard Cochran

- int pdulen, type;
+ int pdulen, type, ret;

I prefer using 'err' instead of 'ret' for this kind of thing.

OK, changed

Post by Richard Cochran

uint8_t *suffix = NULL;
if (cnt < sizeof(struct ptp_header))
- return -1;
+ return -EBADMSG;
- if (hdr_post_recv(&m->header))
- return -1;
+ if (ret = hdr_post_recv(&m->header))
+ return ret;

This pattern triggers the 'suggest parentheses' gcc warning, but
err = hdr_post_recv(&m->header);
if (err)
return err;
Even though it adds an extra line, to me it is still more legible than
if ((ret = hdr_post_recv(&m->header)) != 0)
return ret;

I like this structure more as well, changed

[...]

Post by Richard Cochran

@@ -341,12 +342,12 @@ int msg_post_recv(struct ptp_message *m, int cnt)
if (msg_sots_missing(m)) {
pr_err("received %s without timestamp", msg_type_string(type));
- return -1;
+ return -EBADMSG;

Maybe this case deserves its own code? Then we could move the error
message into port.c, where it could also show the port ID.

That means moving the code to the caller which would result in code
duplication. On the other hand passing struct port * to msg_post_recv just for
the sake of diagnostics does not look right to me either.

Yet, timestamp checking could find its home in transport specific code as the
transport is responsible for timestamping, couldn't it?

Post by Richard Cochran

}
m->tlv_count = suffix_post_recv(suffix, cnt - pdulen, &m->last_tlv);
if (m->tlv_count == -1) {
- return -1;
+ return -EBADMSG;

Here we should also propagate the error codes into suffix_post_recv().

Agree, will implement

Post by Richard Cochran

}
return 0;
diff --git a/pmc_common.c b/pmc_common.c
index ee88bda..cefd038 100644
--- a/pmc_common.c
+++ b/pmc_common.c

[...]

Post by Richard Cochran

@@ -176,8 +177,11 @@ struct ptp_message *pmc_recv(struct pmc *pmc)
if (cnt <= 0) {
pr_err("recv message failed");
goto failed;
- } else if (msg_post_recv(msg, cnt)) {
- pr_err("bad message");
+ } else if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("bad message");
+ else
+ pr_debug("ignoring message");

err = msg_post_recv(msg, cnt);
switch (err) {
...
}

OK

Post by Richard Cochran

goto failed;
}
diff --git a/port.c b/port.c
index a1b17b7..141b8ac 100644
--- a/port.c
+++ b/port.c
@@ -1768,7 +1768,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
{
enum fsm_event event = EV_NONE;
struct ptp_message *msg;
- int cnt, fd = p->fda.fd[fd_index];
+ int cnt, fd = p->fda.fd[fd_index], ret;
switch (fd_index) {
@@ -1813,8 +1813,11 @@ enum fsm_event port_event(struct port *p, int fd_index)
msg_put(msg);
return EV_FAULT_DETECTED;
}
- if (msg_post_recv(msg, cnt)) {
- pr_err("port %hu: bad message", portnum(p));
+ if (ret = msg_post_recv(msg, cnt)) {
+ if (ret == -EBADMSG)
+ pr_err("port %hu: bad message", portnum(p));
+ else
+ pr_debug("port %hu: ignoring message", portnum(p));

Same here, switch/case.

OK, will post updated patch when I'm done with changes.

Thanks for review!

Libor

[Richard Cochran]

Post by Libor Pechacek
[...]

Post by Richard Cochran

Post by Libor Pechacek
@@ -341,12 +342,12 @@ int msg_post_recv(struct ptp_message *m, int cnt)
if (msg_sots_missing(m)) {
pr_err("received %s without timestamp", msg_type_string(type));
- return -1;
+ return -EBADMSG;

Maybe this case deserves its own code? Then we could move the error
message into port.c, where it could also show the port ID.

That means moving the code to the caller which would result in code
duplication. On the other hand passing struct port * to msg_post_recv just for
the sake of diagnostics does not look right to me either.

I meant, move the code and not duplicate it.
Here you could have, for example,

if (msg_sots_missing(m))
return ETIME;

and then, in port.c, something like

err = msg_post_recv(msg, cnt);
switch (err) {
case EPROTO:
pr_debug("port %hu: ignoring message with wrong version",
portnum(p));
break;
case ETIME:
pr_err("port %hu: received %s without timestamp",
portnum(p), msg_type_string(m->type));
break;
case EBADMSG:
pr_err("port %hu: bad message", portnum(p));
break;
}

Post by Libor Pechacek
Yet, timestamp checking could find its home in transport specific code as the
transport is responsible for timestamping, couldn't it?

Whether a time stamp is expected depends on the message type, and I
think it would not be good if the transport should know about the PTP
message details.

Thanks,
Richard

[Libor Pechacek]

Post by Richard Cochran

Post by Libor Pechacek
[...]

Post by Richard Cochran

Post by Libor Pechacek
@@ -341,12 +342,12 @@ int msg_post_recv(struct ptp_message *m, int cnt)
if (msg_sots_missing(m)) {
pr_err("received %s without timestamp", msg_type_string(type));
- return -1;
+ return -EBADMSG;

Maybe this case deserves its own code? Then we could move the error
message into port.c, where it could also show the port ID.

That means moving the code to the caller which would result in code
duplication. On the other hand passing struct port * to msg_post_recv just for
the sake of diagnostics does not look right to me either.

I meant, move the code and not duplicate it.
Here you could have, for example,
if (msg_sots_missing(m))
return ETIME;
and then, in port.c, something like
err = msg_post_recv(msg, cnt);
switch (err) {
pr_debug("port %hu: ignoring message with wrong version",
portnum(p));
break;
pr_err("port %hu: received %s without timestamp",
portnum(p), msg_type_string(m->type));
break;
pr_err("port %hu: bad message", portnum(p));
break;
}

Ah, got it! Thanks.

Post by Richard Cochran

Post by Libor Pechacek
Yet, timestamp checking could find its home in transport specific code as the
transport is responsible for timestamping, couldn't it?

Whether a time stamp is expected depends on the message type, and I
think it would not be good if the transport should know about the PTP
message details.

Understood.

Libor

[Dale Smith]

Post by Libor Pechacek
Hello,
I've played with linuxptp in a network where was a host talking PTPv1. All the
announcements the other host sent out were reported as "bad message" by ptp4l
which was pretty annoying.
The question is how to deal with this situation. msg_post_recv() returns zero
if all is OK, and -1 for any other condition. We need to distinguish between
ignored and malformed packets here, possibly with more fine-grained diagnostics
of the error for better field debugging. One way would be to define our own
set of error conditions, another is to re-use the standard set of error numbers
and give them new meaning in the PTP context.
I'm in favor of trying the second approach as drafted below. Opinions?

Oh yes please. Or something like this.

-Dale