Re: [Linuxptp-devel] Regarding linuxPTP static analysis..
On 04/05/2021 10:16, Miroslav Lichvar wrote: > On Tue, May 04, 2021 at 04:43:51PM +0900, 박웅섭 wrote: >> 1.In the text->length=c->desc.userDescription.length part of clock.c line >> 368, the length declared in the static_ptp_text structure is of type signed >> int and the length declared in the text structure is unsigned int. Why did >> you write the code like this? Assigning Signed integers to unsigned >> integers can lead to overflow problems. > > In my copy of the code the length field of the PTPText structure is > uint8_t. It's a structure used in the network protocol. > Yes, Miroslav is right. PTPText uses unsigned 8 bits as specify in IEEE 1558. As text length is positive or zero. Why should it be a problem? Look on static_ptp_text_set() the length is set using strlen() which is possitive or zero and the maximum length is 255. /* A static_ptp_text is like a PTPText but includes space to store the * text inside the struct. The text array must always be * null-terminated. Also tracks a maximum number of symbols. Note in * UTF-8, # symbols != # bytes. */ #define MAX_PTP_OCTETS 255 I assume static_ptp_text uses int for simlicity. But the length value is bound to unsigned 8 bits, 0 to 255. >> 2. The memcpy function is vulnerable to security. Wouldn't it be correct to >> use memcpy_s instead of memcpy function? > > Isn't that a Windows-only function? > No, C11 https://en.cppreference.com/w/c/string/byte/memcpy However, the make file does not specify standard. Perhaps some users defer C11. Erez ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Announce message generates SDE on MasterOnly port
On Tue, May 04, 2021 at 02:24:07PM +0200, Luigi 'Comio' Mantellini wrote: > As side note, In my testing scenario the master-only ports receive > announce messages and when the slave port loses the signal the node > evolves in grand-master flooding the log with "assuming the grand > master role" message. Not a critical issue, of course... but in this > case we lost time with logging and blocking the main loop. Right, there are a few configurations that result in log spam. It makes sense to inhibit and/or rate limit the spam, but without hurting the "normal" logging. However, it doesn't make sense to fill the code with random tests and logic for bizarre or seldom used configurations. Many of the options from the so-called PTP profiles are poorly designed, but we try to support them as best we can, without gross hackery. > My 2Eurocents. Oh, those are worth 2.44 of my cents! Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Announce message generates SDE on MasterOnly port
Just like other process_*() that quickly return when the port state doesn't require to process the incoming message, we can add a test into process_announce() in order to avoid wasting the cpu. This is not a special case, the announce messages should be ignored on Master-only ports and the ports state cannot change. As side note, In my testing scenario the master-only ports receive announce messages and when the slave port loses the signal the node evolves in grand-master flooding the log with "assuming the grand master role" message. Not a critical issue, of course... but in this case we lost time with logging and blocking the main loop. My 2Eurocents. ciao luigi Il giorno mar 4 mag 2021 alle ore 14:02 Richard Cochran ha scritto: > > On Mon, May 03, 2021 at 06:52:02PM +0200, Luigi 'Comio' Mantellini wrote: > > I noticed that the Announce messages received on MasterOnly ports > > generate a SDE condition in bc_event(). > > and so what? What is the problem? > > > I think that we can return > > EV_NONE when the port is a master_only (or we can skip the > > process_annonce() at all). Is it correct? > > No thanks, I don't want special cases sprinkled all over. > > Thanks, > Richard -- Luigi 'Comio' Mantellini My Professional Profile "UNIX is very simple, it just needs a genius to understand its simplicity." [cit.] ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Port down and sde
Hi Richard,
not a problem, of course. I'm just speaking about code cleanup.
luigi
Il giorno mar 4 mag 2021 alle ore 14:00 Richard Cochran <
richardcoch...@gmail.com> ha scritto:
> On Mon, May 03, 2021 at 06:45:56PM +0200, Luigi 'Comio' Mantellini wrote:
> > /*
> > * A port going down can affect the BMCA result.
> > * Force a state decision event.
> > */
> > if (p->link_status & LINK_DOWN)
> > clock_set_sde(p->clock, 1);
> >
> > I think that should be removed
>
> Why? What problem does it cause?
>
> > because the calling code look like this:
> >
> > case FD_RTNL:
> > pr_debug("port %hu: received link status notification",
> portnum(p));
> > transport_rtnl_link_status(p->trp, fd, p->name,
> port_link_status, p);
> > if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
> > return EV_FAULT_CLEARED;
> > else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
> > (p->link_status & TS_LABEL_CHANGED))
> > return EV_FAULT_DETECTED;
> > else
> > return EV_NONE;
> >
> > raising a EV_FAULT_DETECT on LINK_DOWN
>
> So the SDE gets triggered twice? Not really a problem.
>
> Thanks,
> Richard
>
--
*Luigi 'Comio' Mantellini*
My Professional Profile
*"UNIX is very simple, it just needs a genius to understand its
simplicity." [cit.]*
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Announce message generates SDE on MasterOnly port
On Mon, May 03, 2021 at 06:52:02PM +0200, Luigi 'Comio' Mantellini wrote: > I noticed that the Announce messages received on MasterOnly ports > generate a SDE condition in bc_event(). and so what? What is the problem? > I think that we can return > EV_NONE when the port is a master_only (or we can skip the > process_annonce() at all). Is it correct? No thanks, I don't want special cases sprinkled all over. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Port down and sde
On Mon, May 03, 2021 at 06:45:56PM +0200, Luigi 'Comio' Mantellini wrote:
> /*
> * A port going down can affect the BMCA result.
> * Force a state decision event.
> */
> if (p->link_status & LINK_DOWN)
> clock_set_sde(p->clock, 1);
>
> I think that should be removed
Why? What problem does it cause?
> because the calling code look like this:
>
> case FD_RTNL:
> pr_debug("port %hu: received link status notification", portnum(p));
> transport_rtnl_link_status(p->trp, fd, p->name, port_link_status, p);
> if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
> return EV_FAULT_CLEARED;
> else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
> (p->link_status & TS_LABEL_CHANGED))
> return EV_FAULT_DETECTED;
> else
> return EV_NONE;
>
> raising a EV_FAULT_DETECT on LINK_DOWN
So the SDE gets triggered twice? Not really a problem.
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Planning release 3.2
On Mon, May 03, 2021 at 05:09:09PM +0200, Michael Walle wrote: > did I miss something or wasn't there a 3.2 release? You didn't miss anything. I have been delayed in pushing out the 3.2 release. Sorry, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)
Hi,
The commands we used for testing below issues are:
For 8275.1 profile:
ptp4l -f config_ptp_8275_1.conf -i sriov1 -i sriov0 -H -m 2 --boundary_clock=1
--slaveOnly=1
phc2sys -a -r -m -R 16 -n 24
For 8275.2 profile:
ptp4l -f config_ptp_8275_2.conf -i sriov1 -i sriov0 -H -m 4 --boundary_clock=1
--slaveOnly=1
phc2sys -a -r -m -R 16 -n 44
Thanks,
Amar B S
-Original Message-
From: Amar Subramanyam
Sent: 04 May 2021 16:21
To: linuxptp-devel@lists.sourceforge.net
Cc: Amar Subramanyam ; Karthikkumar Valoor
; Ramana Reddy
Subject: [PATCH] Sync issues observed when ptp4l is ran with jbod and client
only mode (clientOnly=1 and boundary_clock_jbod=1)
This patch addresses the following issues when ptp4l is ran on multiple ports
with jbod and client only mode (i.e clientOnly=1 and boundary_clock_jbod=1):-
1.SYNCHRONIZATION FAULT occurs at every ANNOUNCE RECEIPT Timeout on LISTENING
port, which leads to PTP port state of SLAVE port to flap between SLAVE and
UNCALIBRATED states continuously.
2.When both ports are receiving announce messages, the port other than SLAVE
port is always in LISTENING state, this results in BMCA algorithm being
triggered at every ANNOUNCE RECEIPT Timeout even though there is no change in
successive announce messages.
3.The port other than SLAVE (LISTENING port) prints an error "port 1: master
state recommended in slave only mode
ptp4l[1205469.356]: port 1: defaultDS.priority1 probably misconfigured"
for every ANNOUNCE RECEIPT Timeout.
4.When the port other than the SLAVE port Stops receiving announce packets,
BMCA is triggered at every ANNOUNCE RECEIPT Timeout indefinitely.
Signed-off-by: Amar Subramanyam
Signed-off-by: Karthikkumar Valoor
Signed-off-by: Ramana Reddy
---
clock.c | 17 +
clock.h | 7 +++
port.c | 16
3 files changed, 40 insertions(+)
diff --git a/clock.c b/clock.c
index e545a9b..aedba6d 100644
--- a/clock.c
+++ b/clock.c
@@ -1870,6 +1870,23 @@ enum servo_state clock_synchronize(struct clock *c,
tmv_t ingress, tmv_t origin)
return state;
}
+int clock_get_client_state(struct clock *c) {
+ struct port *piter;
+
+ if (!clock_slave_only(c)) {
+ return 1;
+ }
+
+ LIST_FOREACH(piter, >ports, list) {
+ enum port_state ps = port_state(piter);
+ if (ps == PS_SLAVE || ps == PS_UNCALIBRATED) {
+ return 0;
+ }
+ }
+ return 1;
+}
+
void clock_sync_interval(struct clock *c, int n) {
int shift;
diff --git a/clock.h b/clock.h
index 845d54f..4779ec9 100644
--- a/clock.h
+++ b/clock.h
@@ -326,6 +326,13 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t
ingress,
tmv_t origin);
/**
+ * Inform if any of the port is in SLAVE state.
+ * @param c The clock instance.
+ * @return Return 0 if any port is in SLAVE state, 1 otherwise.
+ */
+int clock_get_client_state(struct clock *c);
+
+/**
* Inform a slaved clock about the master's sync interval.
* @param c The clock instance.
* @param n The logarithm base two of the sync interval.
diff --git a/port.c b/port.c
index 10bb9e1..7d10bb8 100644
--- a/port.c
+++ b/port.c
@@ -390,6 +390,15 @@ static int add_foreign_master(struct port *p, struct
ptp_message *m)
diff = announce_compare(m, tmp);
}
+ /*
+* In multiport slave only mode, there maybe
+* announce messages on LISTENING port. Re-arm
+* the timer if any other configured port is in SLAVE state
+*/
+ if (p->jbod && !clock_get_client_state(p->clock)) {
+ port_set_announce_tmo(p);
+ }
+
return broke_threshold || diff;
}
@@ -2654,6 +2663,13 @@ static enum fsm_event bc_event(struct port *p, int
fd_index)
port_set_announce_tmo(p);
}
+ /*
+* As one of the port is in SLAVE state stop retriggering BMCA
+*/
+ if (p->jbod && !clock_get_client_state(p->clock)) {
+ port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
+ }
+
delay_req_prune(p);
if (clock_slave_only(p->clock) && p->delayMechanism != DM_P2P &&
port_renew_transport(p)) {
--
1.8.3.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature.
Hi, Based on your comments, we are pulling back this change. For the issues we saw with the clientOnly=1 and boundary_clock_jbod=1, we have sent out a new patch fixing them alone. Thank you for your feedback. Thanks, Amar B S -Original Message- From: Ramana Reddy Sent: 28 April 2021 23:45 To: Richard Cochran Cc: Miroslav Lichvar ; linuxptp-devel@lists.sourceforge.net; Amar Subramanyam Subject: RE: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature. -Original Message- From: Richard Cochran [mailto:richardcoch...@gmail.com] Sent: 27 April 2021 21:47 To: Ramana Reddy Cc: Miroslav Lichvar; linuxptp-devel@lists.sourceforge.net; Amar Subramanyam Subject: Re: [Linuxptp-devel] [PATCH] To support Ordinary Clock-Subordinate/Slave just a bunch of devices(jbod) feature. CAUTION: This email originated from outside of Altiostar. Do not click on links or open attachments unless you recognize the sender and you are sure the content is safe. You will never be asked to reset your Altiostar password via email. On Tue, Apr 27, 2021 at 03:59:05PM +, Ramana Reddy wrote: > As I said, running independent clients defeats the purpose of > BMC algorithm and breaks the ITU-T G.8275.2 Spec compliance. The BMC > algorithm should be run locally on all ports of every ordinary and > boundary clock in a domain. Since it runs continuously, it continually > readapts to changes in the network or the clocks. Pls check section 9.3 in > IEEE1588-2008 Spec for details on BMC algorithm. Also refer to Section 6.7 of > A-BMCA requirements of ITU-T G.8275.2 spec. Did you even read ITU-T G.8275.2 Section 6.7 ? It makes no mention of slaveOnly, but rather masterOnly and localPriority. Ramana> Yes I did and I am not sure if you had gone through it in Ramana> detail. Read through the sections 6.7.1 through 6.7.9 where It talks about the Priority2, clockClass, clockQuality, state decision algorithm, master data set comparison which are all used to Select the Master from Slave perspective. You have not clearly identified a problem with linuxptp WRT G.8275.2. If there is a problem, please state it. Ramana> Although we tried mentioning earlier at high level, seems like Ramana> you still haven't understood. Refer the attached document Ramana> listing the existing issues with jbod_boundary_clock and Client only configuration. See if it helps. We can do a quick 30 mins call if required if you still have questions on the issues identified with G.8275.1/G.8275.2 profile BMCA selection. > > I believe Ordinary Clock can have multiple ports No, your belief is incorrect. The number of ports differentiates OC from BC. In fact, that is the only difference. > Pls refer my earlier reply for why we need to run single ptp4l > instance with multiple ports in OC mode. You _seem_ to be proposing a new and different state machine, not specified by 1588 or G.8275.2. If you want to do that, fine, but please develop some kind of design document that explains both the motivation and the theory of operation. Then, you can provide the implementation in the modular way that does not deface the linuxptp architecture. Hint: p->state_machine = clock_slave_only(clock) ? ptp_slave_fsm : ptp_fsm; Ramana> Our intention is not to propose anything outside Ramana> G.8275.1/G.8275.2/IEEE 1588-2008 spec but try to align with what is mentioned in the spec and fix the issues breaking the spec . Probably I might need a separate discussion on this if we agree on existing issues and then take care of comments in the patch as few of them valid comments like not to add any special handling in PTP stack. Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1)
This patch addresses the following issues when ptp4l is ran on multiple ports
with jbod and client only mode (i.e clientOnly=1 and boundary_clock_jbod=1):-
1.SYNCHRONIZATION FAULT occurs at every ANNOUNCE RECEIPT Timeout on LISTENING
port,
which leads to PTP port state of SLAVE port to flap between SLAVE and
UNCALIBRATED
states continuously.
2.When both ports are receiving announce messages, the port other than SLAVE
port
is always in LISTENING state, this results in BMCA algorithm being triggered at
every ANNOUNCE RECEIPT Timeout even though there is no change in successive
announce messages.
3.The port other than SLAVE (LISTENING port) prints an error
"port 1: master state recommended in slave only mode
ptp4l[1205469.356]: port 1: defaultDS.priority1 probably misconfigured"
for every ANNOUNCE RECEIPT Timeout.
4.When the port other than the SLAVE port Stops receiving announce packets,
BMCA is triggered
at every ANNOUNCE RECEIPT Timeout indefinitely.
Signed-off-by: Amar Subramanyam
Signed-off-by: Karthikkumar Valoor
Signed-off-by: Ramana Reddy
---
clock.c | 17 +
clock.h | 7 +++
port.c | 16
3 files changed, 40 insertions(+)
diff --git a/clock.c b/clock.c
index e545a9b..aedba6d 100644
--- a/clock.c
+++ b/clock.c
@@ -1870,6 +1870,23 @@ enum servo_state clock_synchronize(struct clock *c,
tmv_t ingress, tmv_t origin)
return state;
}
+int clock_get_client_state(struct clock *c)
+{
+ struct port *piter;
+
+ if (!clock_slave_only(c)) {
+ return 1;
+ }
+
+ LIST_FOREACH(piter, >ports, list) {
+ enum port_state ps = port_state(piter);
+ if (ps == PS_SLAVE || ps == PS_UNCALIBRATED) {
+ return 0;
+ }
+ }
+ return 1;
+}
+
void clock_sync_interval(struct clock *c, int n)
{
int shift;
diff --git a/clock.h b/clock.h
index 845d54f..4779ec9 100644
--- a/clock.h
+++ b/clock.h
@@ -326,6 +326,13 @@ enum servo_state clock_synchronize(struct clock *c, tmv_t
ingress,
tmv_t origin);
/**
+ * Inform if any of the port is in SLAVE state.
+ * @param c The clock instance.
+ * @return Return 0 if any port is in SLAVE state, 1 otherwise.
+ */
+int clock_get_client_state(struct clock *c);
+
+/**
* Inform a slaved clock about the master's sync interval.
* @param c The clock instance.
* @param n The logarithm base two of the sync interval.
diff --git a/port.c b/port.c
index 10bb9e1..7d10bb8 100644
--- a/port.c
+++ b/port.c
@@ -390,6 +390,15 @@ static int add_foreign_master(struct port *p, struct
ptp_message *m)
diff = announce_compare(m, tmp);
}
+ /*
+* In multiport slave only mode, there maybe
+* announce messages on LISTENING port. Re-arm
+* the timer if any other configured port is in SLAVE state
+*/
+ if (p->jbod && !clock_get_client_state(p->clock)) {
+ port_set_announce_tmo(p);
+ }
+
return broke_threshold || diff;
}
@@ -2654,6 +2663,13 @@ static enum fsm_event bc_event(struct port *p, int
fd_index)
port_set_announce_tmo(p);
}
+ /*
+* As one of the port is in SLAVE state stop retriggering BMCA
+*/
+ if (p->jbod && !clock_get_client_state(p->clock)) {
+ port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
+ }
+
delay_req_prune(p);
if (clock_slave_only(p->clock) && p->delayMechanism != DM_P2P &&
port_renew_transport(p)) {
--
1.8.3.1
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] Regarding linuxPTP static analysis..
On Tue, May 04, 2021 at 04:43:51PM +0900, 박웅섭 wrote: > 1.In the text->length=c->desc.userDescription.length part of clock.c line > 368, the length declared in the static_ptp_text structure is of type signed > int and the length declared in the text structure is unsigned int. Why did > you write the code like this? Assigning Signed integers to unsigned > integers can lead to overflow problems. In my copy of the code the length field of the PTPText structure is uint8_t. It's a structure used in the network protocol. > 2. The memcpy function is vulnerable to security. Wouldn't it be correct to > use memcpy_s instead of memcpy function? Isn't that a Windows-only function? -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] Regarding linuxPTP static analysis..
Hi, I did a static analysis of linuxPTP. Among them, the violation alarm that occurred for the clock.c source code was analyzed and corrected from the viewpoint of security. I have questions among them, so I send an email. 1.In the text->length=c->desc.userDescription.length part of clock.c line 368, the length declared in the static_ptp_text structure is of type signed int and the length declared in the text structure is unsigned int. Why did you write the code like this? Assigning Signed integers to unsigned integers can lead to overflow problems. 2. The memcpy function is vulnerable to security. Wouldn't it be correct to use memcpy_s instead of memcpy function? Thanks. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
