Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
On Sat, Jan 23, 2021 at 05:41:12AM -0800, Richard Cochran wrote: > We'll have to do something about the port numbering. The "real" ports > must start with 1, 2, 3, ... as this is part of the standard. > > Also, there is some code in port_open() and maybe elsewhere that > treats zero as a special case (meaning UDS), for example: I guess we should make that more clear. > Maybe we can have two [!] ports with number zero? It seems to work, but I suspect it might be confusing, especically if there are faults reported for one of the ports. I'll send the whole patchset for review. Thanks, -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
On Wed, Jan 20, 2021 at 04:15:21PM +0100, Miroslav Lichvar wrote:
> Add a second UDS port to allow unprivileged applications to monitor
> ptp4l. On this "read-only" port, disable non-GET actions, forwarding,
> and access to subscriptions.
>
> Ignore non-management messages on both UDS ports to prevent them from
> changing the clock or port state. (This should be a separate patch?)
Yes, please!
> TODO:
> - add option to configure the UDS address (default /var/run/ptp4ro?)
> - chmod(0666) ?
I think this sort of policy can be left to the admin via scripting.
> - update man page
>
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 122 +++-
> port.c | 3 ++
> 2 files changed, 89 insertions(+), 36 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 08c61eb..475aa3d 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -95,10 +95,11 @@ struct clock {
> struct foreign_clock *best;
> struct ClockIdentity best_id;
> LIST_HEAD(ports_head, port) ports;
> - struct port *uds_port;
> + struct port *uds_rw_port;
Part of this series is simply renaming the existing port. It would
ease review to have that in a patch by itself.
> @@ -1173,27 +1201,36 @@ struct clock *clock_create(enum clock_type type,
> struct config *config,
>
> LIST_INIT(>subscribers);
> LIST_INIT(>ports);
> - c->last_port_number = 0;
> + c->last_port_number = -1;
>
> if (clock_resize_pollfd(c, 0)) {
> pr_err("failed to allocate pollfd");
> return NULL;
> }
>
> - /* Create the UDS interface. */
> - c->uds_port = port_open(phc_device, phc_index, timestamping, 0,
> c->udsif, c);
> - if (!c->uds_port) {
> - pr_err("failed to open the UDS port");
> + /* Create the UDS interfaces. */
> + c->uds_rw_port = port_open(phc_device, phc_index, timestamping,
> +++c->last_port_number, c->uds_rw_if, c);
> + if (!c->uds_rw_port) {
> + pr_err("failed to open the UDS RW port");
> return NULL;
> }
> clock_fda_changed(c);
>
> - c->slave_event_monitor = monitor_create(config, c->uds_port);
> + c->slave_event_monitor = monitor_create(config, c->uds_rw_port);
> if (!c->slave_event_monitor) {
> pr_err("failed to create slave event monitor");
> return NULL;
> }
>
> + c->uds_ro_port = port_open(phc_device, phc_index, timestamping,
> +++c->last_port_number, c->uds_ro_if, c);
We'll have to do something about the port numbering. The "real" ports
must start with 1, 2, 3, ... as this is part of the standard.
Also, there is some code in port_open() and maybe elsewhere that
treats zero as a special case (meaning UDS), for example:
p->fault_fd = -1;
if (number) {
p->fault_fd = timerfd_create(CLOCK_MONOTONIC, 0);
...
}
Maybe we can have two [!] ports with number zero?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
On 1/21/2021 12:31 AM, Miroslav Lichvar wrote: > On Wed, Jan 20, 2021 at 10:13:25PM +, Keller, Jacob E wrote: >> It makes sense to remove forwarding, but I am not sure I understand the >> justification for removing access to subscriptions.. if the subscription is >> for read only data, why doesn't it make sense to allow that over the read >> only socket? > > The subscription itself requires some state. If we say the new socket > is safe to be accessed by untrusted applications, we need to be really > sure they cannot do anything bad, e.g. create a large number of > subscriptions to crash ptp4l or break subscriptions on the "rw" > socket. Such issues would become security issues. > > There might be a way to provide subscriptions on the "ro" socket, but > they need to be separate from the "rw" subscriptions and have some > limiting implemented. > > I'd like to start with a minimal feature set that we can be > comfortable with and maybe add other features later if there is a > demand for them. > Right. This makes sense. We can obviously extend the RO sockets in the future, but I think it makes sense to limit it. To me, it seems good to have some condensed version of this explanation in the commit message or somewhere, since it may not be obvious why it is limited to those on the outside. ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
On Wed, Jan 20, 2021 at 10:13:25PM +, Keller, Jacob E wrote: > It makes sense to remove forwarding, but I am not sure I understand the > justification for removing access to subscriptions.. if the subscription is > for read only data, why doesn't it make sense to allow that over the read > only socket? The subscription itself requires some state. If we say the new socket is safe to be accessed by untrusted applications, we need to be really sure they cannot do anything bad, e.g. create a large number of subscriptions to crash ptp4l or break subscriptions on the "rw" socket. Such issues would become security issues. There might be a way to provide subscriptions on the "ro" socket, but they need to be separate from the "rw" subscriptions and have some limiting implemented. I'd like to start with a minimal feature set that we can be comfortable with and maybe add other features later if there is a demand for them. -- Miroslav Lichvar ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Wednesday, January 20, 2021 7:15 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for
> monitoring.
>
> Add a second UDS port to allow unprivileged applications to monitor
> ptp4l. On this "read-only" port, disable non-GET actions, forwarding,
> and access to subscriptions.
>
It makes sense to remove forwarding, but I am not sure I understand the
justification for removing access to subscriptions.. if the subscription is for
read only data, why doesn't it make sense to allow that over the read only
socket?
Thanks,
Jake
> Ignore non-management messages on both UDS ports to prevent them from
> changing the clock or port state. (This should be a separate patch?)
>
> TODO:
> - add option to configure the UDS address (default /var/run/ptp4ro?)
> - chmod(0666) ?
> - update man page
>
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 122 +++-
> port.c | 3 ++
> 2 files changed, 89 insertions(+), 36 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 08c61eb..475aa3d 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -95,10 +95,11 @@ struct clock {
> struct foreign_clock *best;
> struct ClockIdentity best_id;
> LIST_HEAD(ports_head, port) ports;
> - struct port *uds_port;
> + struct port *uds_rw_port;
> + struct port *uds_ro_port;
> struct pollfd *pollfd;
> int pollfd_valid;
> - int nports; /* does not include the UDS port */
> + int nports; /* does not include the two UDS ports */
> int last_port_number;
> int sde;
> int free_running;
> @@ -129,7 +130,8 @@ struct clock {
> struct clock_stats stats;
> int stats_interval;
> struct clockcheck *sanity_check;
> - struct interface *udsif;
> + struct interface *uds_rw_if;
> + struct interface *uds_ro_if;
> LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
> struct monitor *slave_event_monitor;
> };
> @@ -245,7 +247,7 @@ void clock_send_notification(struct clock *c, struct
> ptp_message *msg,
> {
> unsigned int event_pos = event / 8;
> uint8_t mask = 1 << (event % 8);
> - struct port *uds = c->uds_port;
> + struct port *uds = c->uds_rw_port;
> struct clock_subscriber *s;
>
> LIST_FOREACH(s, >subscribers, list) {
> @@ -267,13 +269,15 @@ void clock_destroy(struct clock *c)
> {
> struct port *p, *tmp;
>
> - interface_destroy(c->udsif);
> + interface_destroy(c->uds_rw_if);
> + interface_destroy(c->uds_ro_if);
> clock_flush_subscriptions(c);
> LIST_FOREACH_SAFE(p, >ports, list, tmp) {
> clock_remove_port(c, p);
> }
> monitor_destroy(c->slave_event_monitor);
> - port_close(c->uds_port);
> + port_close(c->uds_rw_port);
> + port_close(c->uds_ro_port);
> free(c->pollfd);
> if (c->clkid != CLOCK_REALTIME) {
> phc_close(c->clkid);
> @@ -442,8 +446,8 @@ static int clock_management_fill_response(struct clock
> *c, struct port *p,
> datalen = sizeof(*gsn);
> break;
> case TLV_SUBSCRIBE_EVENTS_NP:
> - if (p != c->uds_port) {
> - /* Only the UDS port allowed. */
> + if (p != c->uds_rw_port) {
> + /* Only the UDS RW port allowed. */
> break;
> }
> sen = (struct subscribe_events_np *)tlv->data;
> @@ -774,6 +778,10 @@ static int clock_utc_correct(struct clock *c, tmv_t
> ingress)
> static int forwarding(struct clock *c, struct port *p)
> {
> enum port_state ps = port_state(p);
> +
> + if (p == c->uds_ro_port)
> + return 0;
> +
> switch (ps) {
> case PS_MASTER:
> case PS_GRAND_MASTER:
> @@ -784,7 +792,7 @@ static int forwarding(struct clock *c, struct port *p)
> default:
> break;
> }
> - if (p == c->uds_port && ps != PS_FAULTY) {
> + if (p == c->uds_rw_port && ps != PS_FAULTY) {
> return 1;
> }
> return 0;
> @@ -818,7 +826,7 @@ static int clock_add_port(struct clock *c, const char
> *phc_device,
> {
> struct port *p, *piter, *lastp = NULL;
>
> - if (clock_resize_pollfd(c, c->nports + 1)) {
> + if (clock_resize_pollfd(c, c->nports + 2)) {
> return -1;
> }
> p = port_open(phc_device, p
[Linuxptp-devel] [RFC PATCH] clock: Add read-only UDS port for monitoring.
Add a second UDS port to allow unprivileged applications to monitor
ptp4l. On this "read-only" port, disable non-GET actions, forwarding,
and access to subscriptions.
Ignore non-management messages on both UDS ports to prevent them from
changing the clock or port state. (This should be a separate patch?)
TODO:
- add option to configure the UDS address (default /var/run/ptp4ro?)
- chmod(0666) ?
- update man page
Signed-off-by: Miroslav Lichvar
---
clock.c | 122 +++-
port.c | 3 ++
2 files changed, 89 insertions(+), 36 deletions(-)
diff --git a/clock.c b/clock.c
index 08c61eb..475aa3d 100644
--- a/clock.c
+++ b/clock.c
@@ -95,10 +95,11 @@ struct clock {
struct foreign_clock *best;
struct ClockIdentity best_id;
LIST_HEAD(ports_head, port) ports;
- struct port *uds_port;
+ struct port *uds_rw_port;
+ struct port *uds_ro_port;
struct pollfd *pollfd;
int pollfd_valid;
- int nports; /* does not include the UDS port */
+ int nports; /* does not include the two UDS ports */
int last_port_number;
int sde;
int free_running;
@@ -129,7 +130,8 @@ struct clock {
struct clock_stats stats;
int stats_interval;
struct clockcheck *sanity_check;
- struct interface *udsif;
+ struct interface *uds_rw_if;
+ struct interface *uds_ro_if;
LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers;
struct monitor *slave_event_monitor;
};
@@ -245,7 +247,7 @@ void clock_send_notification(struct clock *c, struct
ptp_message *msg,
{
unsigned int event_pos = event / 8;
uint8_t mask = 1 << (event % 8);
- struct port *uds = c->uds_port;
+ struct port *uds = c->uds_rw_port;
struct clock_subscriber *s;
LIST_FOREACH(s, >subscribers, list) {
@@ -267,13 +269,15 @@ void clock_destroy(struct clock *c)
{
struct port *p, *tmp;
- interface_destroy(c->udsif);
+ interface_destroy(c->uds_rw_if);
+ interface_destroy(c->uds_ro_if);
clock_flush_subscriptions(c);
LIST_FOREACH_SAFE(p, >ports, list, tmp) {
clock_remove_port(c, p);
}
monitor_destroy(c->slave_event_monitor);
- port_close(c->uds_port);
+ port_close(c->uds_rw_port);
+ port_close(c->uds_ro_port);
free(c->pollfd);
if (c->clkid != CLOCK_REALTIME) {
phc_close(c->clkid);
@@ -442,8 +446,8 @@ static int clock_management_fill_response(struct clock *c,
struct port *p,
datalen = sizeof(*gsn);
break;
case TLV_SUBSCRIBE_EVENTS_NP:
- if (p != c->uds_port) {
- /* Only the UDS port allowed. */
+ if (p != c->uds_rw_port) {
+ /* Only the UDS RW port allowed. */
break;
}
sen = (struct subscribe_events_np *)tlv->data;
@@ -774,6 +778,10 @@ static int clock_utc_correct(struct clock *c, tmv_t
ingress)
static int forwarding(struct clock *c, struct port *p)
{
enum port_state ps = port_state(p);
+
+ if (p == c->uds_ro_port)
+ return 0;
+
switch (ps) {
case PS_MASTER:
case PS_GRAND_MASTER:
@@ -784,7 +792,7 @@ static int forwarding(struct clock *c, struct port *p)
default:
break;
}
- if (p == c->uds_port && ps != PS_FAULTY) {
+ if (p == c->uds_rw_port && ps != PS_FAULTY) {
return 1;
}
return 0;
@@ -818,7 +826,7 @@ static int clock_add_port(struct clock *c, const char
*phc_device,
{
struct port *p, *piter, *lastp = NULL;
- if (clock_resize_pollfd(c, c->nports + 1)) {
+ if (clock_resize_pollfd(c, c->nports + 2)) {
return -1;
}
p = port_open(phc_device, phc_index, timestamping,
@@ -1044,20 +1052,40 @@ struct clock *clock_create(enum clock_type type, struct
config *config,
/* Configure the UDS. */
uds_ifname = config_get_string(config, NULL, "uds_address");
- c->udsif = interface_create(uds_ifname);
- if (config_set_section_int(config, interface_name(c->udsif),
+ c->uds_rw_if = interface_create(uds_ifname);
+ if (config_set_section_int(config, interface_name(c->uds_rw_if),
"announceReceiptTimeout", 0)) {
return NULL;
}
- if (config_set_section_int(config, interface_name(c->udsif),
+ if (config_set_section_int(config, interface_name(c->uds_rw_if),
"delay_mechanism", DM_AUTO)) {
return NULL;
}
- if (config_set_section_int(config, interface_name(c->udsif),
+ if (config_set_section_int(config, interface_name(c->uds_rw_if),
"network_transport", TRANS_UDS)) {
