Post by Richard CochranPost by Delio BrignoliI added the 'same sequence number' test because p->peer_delay_resp
was not freed and set to NULL when sending a new peer delay request
and I wanted to be sure the duplicated response had been generated
in response to the current request. We now free p->peer_delay_resp
when sending a new request so I can drop the 'same_sid' part of the
test.
Then I guess I applied your patch titled
Free peer delay responses and followup messages when sending a new
peer delay request
too quickly. We do need to remember the peer port ID from the last
exchange. If the network changes between our delay requests, then we
need to reset the port.
You had said in that patch,
If messages are not freed, it is possible (with purposely crafted
traffic) to trigger a peer delay calculation which will use
message's data from the previous round.
and I took your word for it, but I did not think it through to
understand what you are trying to fix. Anyhow, I don't doubt that you
found an real issue.
When I posted the patch you mention above I didn't realize this error
condition:
if (p->peer_delay_resp) {
if (!source_pid_eq(p->peer_delay_resp, m)) {
pr_err("port %hu: multiple peer responses", portnum(p));
return -1;
}
}
would also be triggered when the peer changed between delay requests.
Post by Richard Cochran1. Multiple peer delay responses. If we receive more than one
response, this is always an error. It does not matter what the port
IDs or sequence numbers are (although you might want to
differentiate the exact kind of error).
2. Change of peer port ID. This is also an error, but it might simply
mean that the network changed, like someone unpluged one node and
plugged into another. Handling this error should result in
resetting the port, including the learned delay.
Now, after your last two patches, I think case number 2 is not being
handled any more.
You are correct, case 2 is not being handled anymore.
Post by Richard CochranOne way to fix this would be to remember the peer port ID in the
'struct port'. We could then check that the reverse exchange (when the
peer sends us a delay request) is also consistent.
Does the patch below look like a viable solution? If yes, I will submit
it as a standalone email. Note that the port is learning the peer's
port-id when processing either a pdelay req or a pdelay resp (whichever
happens first).
---
port.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/port.c b/port.c
index 2f50888..b3e6e5a 100644
--- a/port.c
+++ b/port.c
@@ -60,6 +60,8 @@ struct port {
struct ptp_message *peer_delay_req;
struct ptp_message *peer_delay_resp;
struct ptp_message *peer_delay_fup;
+ int peer_portid_valid;
+ struct PortIdentity peer_portid;
struct {
UInteger16 announce;
UInteger16 delayreq;
@@ -679,6 +681,8 @@ static void port_nrate_initialize(struct port *p)
/* We start in the 'incapable' state. */
p->pdr_missing = ALLOWED_LOST_RESPONSES + 1;
+ p->peer_portid_valid = 0;
+
p->nrate.origin1 = tmv_zero();
p->nrate.ingress1 = tmv_zero();
p->nrate.max_count = (1 << shift);
@@ -1344,6 +1348,15 @@ static int process_pdelay_req(struct port *p,
struct ptp_message *m)
pr_info("port %hu: peer detected, switch to P2P",
portnum(p));
p->delayMechanism = DM_P2P;
}
+ if (p->peer_portid_valid) {
+ if (!pid_eq(&p->peer_portid,
&m->header.sourcePortIdentity)) {
+ pr_err("port %hu: peer port id changed",
portnum(p));
+ return -1;
+ }
+ } else {
+ p->peer_portid_valid = 1;
+ p->peer_portid = m->header.sourcePortIdentity;
+ }
rsp = msg_allocate();
if (!rsp)
@@ -1507,6 +1520,15 @@ static int process_pdelay_resp(struct port *p,
struct ptp_message *m)
pr_err("port %hu: rogue peer delay response", portnum(p));
return -1;
}
+ if (p->peer_portid_valid) {
+ if (!pid_eq(&p->peer_portid,
&m->header.sourcePortIdentity)) {
+ pr_err("port %hu: peer port id changed",
portnum(p));
+ return -1;
+ }
+ } else {
+ p->peer_portid_valid = 1;
+ p->peer_portid = m->header.sourcePortIdentity;
+ }
if (p->peer_delay_resp) {
msg_put(p->peer_delay_resp);
--
1.7.0.4