Re: [PATCH] connector: fix out-of-order cn_proc netlink message delivery

[Evgeniy Polyakov]

Hi Aaron

24.06.2016, 16:07, "Aaron Campbell" :
> The proc connector messages include a sequence number, allowing userspace
> programs to detect lost messages. However, performing this detection is
> currently more difficult than necessary, since netlink messages can be
> delivered to the application out-of-order. To fix this, leave pre-emption
> disabled during cn_netlink_send(), and use GFP_NOWAIT.
>
> The following was written as a test case. Building the kernel w/ make -j32
> proved a reliable way to generate out-of-order cn_proc messages.

This is not actually about out-of-order sending which is impossible iirc,
but the way fork pushes messages into socket queue in parallel. What you've done
is syncing one more layer higher.

I'm not against this patch if you think it does fix some issues, but wording is 
not correct imo.

Re: [PATCH] connector: fix out-of-order cn_proc netlink message delivery

[Evgeniy Polyakov]

Hi Aaron

24.06.2016, 16:07, "Aaron Campbell" :
> The proc connector messages include a sequence number, allowing userspace
> programs to detect lost messages. However, performing this detection is
> currently more difficult than necessary, since netlink messages can be
> delivered to the application out-of-order. To fix this, leave pre-emption
> disabled during cn_netlink_send(), and use GFP_NOWAIT.
>
> The following was written as a test case. Building the kernel w/ make -j32
> proved a reliable way to generate out-of-order cn_proc messages.

This is not actually about out-of-order sending which is impossible iirc,
but the way fork pushes messages into socket queue in parallel. What you've done
is syncing one more layer higher.

I'm not against this patch if you think it does fix some issues, but wording is 
not correct imo.

Re: [PATCH] connector: fix out-of-order cn_proc netlink message delivery

[David Miller]

From: Aaron Campbell 
Date: Fri, 24 Jun 2016 10:05:32 -0300

> The proc connector messages include a sequence number, allowing userspace
> programs to detect lost messages.  However, performing this detection is
> currently more difficult than necessary, since netlink messages can be
> delivered to the application out-of-order.  To fix this, leave pre-emption
> disabled during cn_netlink_send(), and use GFP_NOWAIT.
> 
> The following was written as a test case.  Building the kernel w/ make -j32
> proved a reliable way to generate out-of-order cn_proc messages.
 ...
> Signed-off-by: Aaron Campbell 

Applied.

Re: [PATCH] connector: fix out-of-order cn_proc netlink message delivery

[David Miller]

From: Aaron Campbell 
Date: Fri, 24 Jun 2016 10:05:32 -0300

> The proc connector messages include a sequence number, allowing userspace
> programs to detect lost messages.  However, performing this detection is
> currently more difficult than necessary, since netlink messages can be
> delivered to the application out-of-order.  To fix this, leave pre-emption
> disabled during cn_netlink_send(), and use GFP_NOWAIT.
> 
> The following was written as a test case.  Building the kernel w/ make -j32
> proved a reliable way to generate out-of-order cn_proc messages.
 ...
> Signed-off-by: Aaron Campbell 

Applied.

[PATCH] connector: fix out-of-order cn_proc netlink message delivery

[Aaron Campbell]

The proc connector messages include a sequence number, allowing userspace
programs to detect lost messages.  However, performing this detection is
currently more difficult than necessary, since netlink messages can be
delivered to the application out-of-order.  To fix this, leave pre-emption
disabled during cn_netlink_send(), and use GFP_NOWAIT.

The following was written as a test case.  Building the kernel w/ make -j32
proved a reliable way to generate out-of-order cn_proc messages.

int
main(int argc, char *argv[])
{
static uint32_t last_seq[CPU_SETSIZE], seq;
int cpu, fd;
struct sockaddr_nl sa;
struct __attribute__((aligned(NLMSG_ALIGNTO))) {
struct nlmsghdr nl_hdr;
struct __attribute__((__packed__)) {
struct cn_msg cn_msg;
struct proc_event cn_proc;
};
} rmsg;
struct __attribute__((aligned(NLMSG_ALIGNTO))) {
struct nlmsghdr nl_hdr;
struct __attribute__((__packed__)) {
struct cn_msg cn_msg;
enum proc_cn_mcast_op cn_mcast;
};
} smsg;

fd = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
if (fd < 0) {
perror("socket");
}

sa.nl_family = AF_NETLINK;
sa.nl_groups = CN_IDX_PROC;
sa.nl_pid = getpid();
if (bind(fd, (struct sockaddr *), sizeof(sa)) < 0) {
perror("bind");
}

memset(, 0, sizeof(smsg));
smsg.nl_hdr.nlmsg_len = sizeof(smsg);
smsg.nl_hdr.nlmsg_pid = getpid();
smsg.nl_hdr.nlmsg_type = NLMSG_DONE;
smsg.cn_msg.id.idx = CN_IDX_PROC;
smsg.cn_msg.id.val = CN_VAL_PROC;
smsg.cn_msg.len = sizeof(enum proc_cn_mcast_op);
smsg.cn_mcast = PROC_CN_MCAST_LISTEN;
if (send(fd, , sizeof(smsg), 0) != sizeof(smsg)) {
perror("send");
}

while (recv(fd, , sizeof(rmsg), 0) == sizeof(rmsg)) {
cpu = rmsg.cn_proc.cpu;
if (cpu < 0) {
continue;
}
seq = rmsg.cn_msg.seq;
if ((last_seq[cpu] != 0) && (seq != last_seq[cpu] + 1)) {
printf("out-of-order seq=%d on cpu=%d\n", seq, cpu);
}
last_seq[cpu] = seq;
}

/* NOTREACHED */

perror("recv");

return -1;
}

Signed-off-by: Aaron Campbell 
---
 drivers/connector/cn_proc.c | 43 ++-
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 15d06fc..b02f9c6 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -56,11 +56,21 @@ static struct cb_id cn_proc_event_id = { CN_IDX_PROC, 
CN_VAL_PROC };
 /* proc_event_counts is used as the sequence number of the netlink message */
 static DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };
 
-static inline void get_seq(__u32 *ts, int *cpu)
+static inline void send_msg(struct cn_msg *msg)
 {
preempt_disable();
-   *ts = __this_cpu_inc_return(proc_event_counts) - 1;
-   *cpu = smp_processor_id();
+
+   msg->seq = __this_cpu_inc_return(proc_event_counts) - 1;
+   ((struct proc_event *)msg->data)->cpu = smp_processor_id();
+
+   /*
+* Preemption remains disabled during send to ensure the messages are
+* ordered according to their sequence numbers.
+*
+* If cn_netlink_send() fails, the data is not sent.
+*/
+   cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_NOWAIT);
+
preempt_enable();
 }
 
@@ -77,7 +87,6 @@ void proc_fork_connector(struct task_struct *task)
msg = buffer_to_cn_msg(buffer);
ev = (struct proc_event *)msg->data;
memset(>event_data, 0, sizeof(ev->event_data));
-   get_seq(>seq, >cpu);
ev->timestamp_ns = ktime_get_ns();
ev->what = PROC_EVENT_FORK;
rcu_read_lock();
@@ -92,8 +101,7 @@ void proc_fork_connector(struct task_struct *task)
msg->ack = 0; /* not used */
msg->len = sizeof(*ev);
msg->flags = 0; /* not used */
-   /*  If cn_netlink_send() failed, the data is not sent */
-   cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_KERNEL);
+   send_msg(msg);
 }
 
 void proc_exec_connector(struct task_struct *task)
@@ -108,7 +116,6 @@ void proc_exec_connector(struct task_struct *task)
msg = buffer_to_cn_msg(buffer);
ev = (struct proc_event *)msg->data;
memset(>event_data, 0, sizeof(ev->event_data));
-   get_seq(>seq, >cpu);
ev->timestamp_ns = ktime_get_ns();
ev->what = PROC_EVENT_EXEC;
ev->event_data.exec.process_pid = task->pid;
@@ -118,7 +125,7 @@ void proc_exec_connector(struct task_struct *task)
msg->ack = 0; /* not used */
msg->len = 

[PATCH] connector: fix out-of-order cn_proc netlink message delivery

[Aaron Campbell]

The proc connector messages include a sequence number, allowing userspace
programs to detect lost messages.  However, performing this detection is
currently more difficult than necessary, since netlink messages can be
delivered to the application out-of-order.  To fix this, leave pre-emption
disabled during cn_netlink_send(), and use GFP_NOWAIT.

The following was written as a test case.  Building the kernel w/ make -j32
proved a reliable way to generate out-of-order cn_proc messages.

int
main(int argc, char *argv[])
{
static uint32_t last_seq[CPU_SETSIZE], seq;
int cpu, fd;
struct sockaddr_nl sa;
struct __attribute__((aligned(NLMSG_ALIGNTO))) {
struct nlmsghdr nl_hdr;
struct __attribute__((__packed__)) {
struct cn_msg cn_msg;
struct proc_event cn_proc;
};
} rmsg;
struct __attribute__((aligned(NLMSG_ALIGNTO))) {
struct nlmsghdr nl_hdr;
struct __attribute__((__packed__)) {
struct cn_msg cn_msg;
enum proc_cn_mcast_op cn_mcast;
};
} smsg;

fd = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR);
if (fd < 0) {
perror("socket");
}

sa.nl_family = AF_NETLINK;
sa.nl_groups = CN_IDX_PROC;
sa.nl_pid = getpid();
if (bind(fd, (struct sockaddr *), sizeof(sa)) < 0) {
perror("bind");
}

memset(, 0, sizeof(smsg));
smsg.nl_hdr.nlmsg_len = sizeof(smsg);
smsg.nl_hdr.nlmsg_pid = getpid();
smsg.nl_hdr.nlmsg_type = NLMSG_DONE;
smsg.cn_msg.id.idx = CN_IDX_PROC;
smsg.cn_msg.id.val = CN_VAL_PROC;
smsg.cn_msg.len = sizeof(enum proc_cn_mcast_op);
smsg.cn_mcast = PROC_CN_MCAST_LISTEN;
if (send(fd, , sizeof(smsg), 0) != sizeof(smsg)) {
perror("send");
}

while (recv(fd, , sizeof(rmsg), 0) == sizeof(rmsg)) {
cpu = rmsg.cn_proc.cpu;
if (cpu < 0) {
continue;
}
seq = rmsg.cn_msg.seq;
if ((last_seq[cpu] != 0) && (seq != last_seq[cpu] + 1)) {
printf("out-of-order seq=%d on cpu=%d\n", seq, cpu);
}
last_seq[cpu] = seq;
}

/* NOTREACHED */

perror("recv");

return -1;
}

Signed-off-by: Aaron Campbell 
---
 drivers/connector/cn_proc.c | 43 ++-
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 15d06fc..b02f9c6 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -56,11 +56,21 @@ static struct cb_id cn_proc_event_id = { CN_IDX_PROC, 
CN_VAL_PROC };
 /* proc_event_counts is used as the sequence number of the netlink message */
 static DEFINE_PER_CPU(__u32, proc_event_counts) = { 0 };
 
-static inline void get_seq(__u32 *ts, int *cpu)
+static inline void send_msg(struct cn_msg *msg)
 {
preempt_disable();
-   *ts = __this_cpu_inc_return(proc_event_counts) - 1;
-   *cpu = smp_processor_id();
+
+   msg->seq = __this_cpu_inc_return(proc_event_counts) - 1;
+   ((struct proc_event *)msg->data)->cpu = smp_processor_id();
+
+   /*
+* Preemption remains disabled during send to ensure the messages are
+* ordered according to their sequence numbers.
+*
+* If cn_netlink_send() fails, the data is not sent.
+*/
+   cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_NOWAIT);
+
preempt_enable();
 }
 
@@ -77,7 +87,6 @@ void proc_fork_connector(struct task_struct *task)
msg = buffer_to_cn_msg(buffer);
ev = (struct proc_event *)msg->data;
memset(>event_data, 0, sizeof(ev->event_data));
-   get_seq(>seq, >cpu);
ev->timestamp_ns = ktime_get_ns();
ev->what = PROC_EVENT_FORK;
rcu_read_lock();
@@ -92,8 +101,7 @@ void proc_fork_connector(struct task_struct *task)
msg->ack = 0; /* not used */
msg->len = sizeof(*ev);
msg->flags = 0; /* not used */
-   /*  If cn_netlink_send() failed, the data is not sent */
-   cn_netlink_send(msg, 0, CN_IDX_PROC, GFP_KERNEL);
+   send_msg(msg);
 }
 
 void proc_exec_connector(struct task_struct *task)
@@ -108,7 +116,6 @@ void proc_exec_connector(struct task_struct *task)
msg = buffer_to_cn_msg(buffer);
ev = (struct proc_event *)msg->data;
memset(>event_data, 0, sizeof(ev->event_data));
-   get_seq(>seq, >cpu);
ev->timestamp_ns = ktime_get_ns();
ev->what = PROC_EVENT_EXEC;
ev->event_data.exec.process_pid = task->pid;
@@ -118,7 +125,7 @@ void proc_exec_connector(struct task_struct *task)
msg->ack = 0; /* not used */
msg->len = sizeof(*ev);