| Message ID | 20170103003910.8984-1-wr0112358@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jan 03, 2017 at 01:39:10AM +0100, Reiter Wolfgang wrote: > Final nlmsg_len field update must reflect inserted net_dm_drop_point > data. > > This patch depends on previous patch: > "drop_monitor: add missing call to genlmsg_end" > > Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> > --- > net/core/drop_monitor.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c > index f465bad..fb55327 100644 > --- a/net/core/drop_monitor.c > +++ b/net/core/drop_monitor.c > @@ -102,7 +102,6 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data) > } > msg = nla_data(nla); > memset(msg, 0, al); > - genlmsg_end(skb, msg_header); > goto out; > > err: > @@ -112,6 +111,13 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data) > swap(data->skb, skb); > spin_unlock_irqrestore(&data->lock, flags); > > + if (skb) { > + struct nlmsghdr *nlh = (struct nlmsghdr *)skb->data; > + struct genlmsghdr *gnlh = (struct genlmsghdr *)nlmsg_data(nlh); > + > + genlmsg_end(skb, genlmsg_data(gnlh)); > + } > + > return skb; > } > > -- > 2.9.3 > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
From: Reiter Wolfgang <wr0112358@gmail.com> Date: Tue, 3 Jan 2017 01:39:10 +0100 > Final nlmsg_len field update must reflect inserted net_dm_drop_point > data. > > This patch depends on previous patch: > "drop_monitor: add missing call to genlmsg_end" > > Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> I don't understand why the current code doesn't work properly. All over the tree, the pattern is: x = genlmsg_put(skb, ...); ... genlmsg_end(skb, x); And that is exactly what the code is doing right now.
On Tue, Jan 03, 2017 at 09:54:19AM -0500, David Miller wrote: > From: Reiter Wolfgang <wr0112358@gmail.com> > Date: Tue, 3 Jan 2017 01:39:10 +0100 > > > Final nlmsg_len field update must reflect inserted net_dm_drop_point > > data. > > > > This patch depends on previous patch: > > "drop_monitor: add missing call to genlmsg_end" > > > > Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> > > I don't understand why the current code doesn't work properly. > > All over the tree, the pattern is: > > x = genlmsg_put(skb, ...); > ... > genlmsg_end(skb, x); > > And that is exactly what the code is doing right now. > Because reset_per_cpu_data should close the use of of the established skb that was being written to. Without this patch we add the END tlv to the skb that is just getting started for use in the drop monitor, rather than for the skb that is getting returned for use in sending up to user space listeners. Or am I missing something?
From: Neil Horman <nhorman@tuxdriver.com> Date: Tue, 3 Jan 2017 11:04:43 -0500 > On Tue, Jan 03, 2017 at 09:54:19AM -0500, David Miller wrote: >> From: Reiter Wolfgang <wr0112358@gmail.com> >> Date: Tue, 3 Jan 2017 01:39:10 +0100 >> >> > Final nlmsg_len field update must reflect inserted net_dm_drop_point >> > data. >> > >> > This patch depends on previous patch: >> > "drop_monitor: add missing call to genlmsg_end" >> > >> > Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> >> >> I don't understand why the current code doesn't work properly. >> >> All over the tree, the pattern is: >> >> x = genlmsg_put(skb, ...); >> ... >> genlmsg_end(skb, x); >> >> And that is exactly what the code is doing right now. >> > > Because reset_per_cpu_data should close the use of of the established skb > that was being written to. Without this patch we add the END tlv to the skb > that is just getting started for use in the drop monitor, rather than for the > skb that is getting returned for use in sending up to user space listeners. > > Or am I missing something? That's the critical part I didn't see, thanks for explaining. Applied and queued up for -stabel, thanks.
Yes, genlmsg_end changes nlmsg_len field dependent on skb->tail. After allocation in reset_per_cpu_data skb->tail is modified in trace_drop_common via __nla_reserve_nohdr. Best place for setting nlmsg_len to its final value is after being swapped out in reset_per_cpu_data. Neil Horman <nhorman@tuxdriver.com> writes: > On Tue, Jan 03, 2017 at 09:54:19AM -0500, David Miller wrote: >> From: Reiter Wolfgang <wr0112358@gmail.com> >> Date: Tue, 3 Jan 2017 01:39:10 +0100 >> >> > Final nlmsg_len field update must reflect inserted net_dm_drop_point >> > data. >> > >> > This patch depends on previous patch: >> > "drop_monitor: add missing call to genlmsg_end" >> > >> > Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> >> >> I don't understand why the current code doesn't work properly. >> >> All over the tree, the pattern is: >> >> x = genlmsg_put(skb, ...); >> ... >> genlmsg_end(skb, x); >> >> And that is exactly what the code is doing right now. >> > > Because reset_per_cpu_data should close the use of of the established skb > that was being written to. Without this patch we add the END tlv to the skb > that is just getting started for use in the drop monitor, rather than for the > skb that is getting returned for use in sending up to user space listeners. > > Or am I missing something?
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index f465bad..fb55327 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -102,7 +102,6 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data) } msg = nla_data(nla); memset(msg, 0, al); - genlmsg_end(skb, msg_header); goto out; err: @@ -112,6 +111,13 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data) swap(data->skb, skb); spin_unlock_irqrestore(&data->lock, flags); + if (skb) { + struct nlmsghdr *nlh = (struct nlmsghdr *)skb->data; + struct genlmsghdr *gnlh = (struct genlmsghdr *)nlmsg_data(nlh); + + genlmsg_end(skb, genlmsg_data(gnlh)); + } + return skb; }
Final nlmsg_len field update must reflect inserted net_dm_drop_point data. This patch depends on previous patch: "drop_monitor: add missing call to genlmsg_end" Signed-off-by: Reiter Wolfgang <wr0112358@gmail.com> --- net/core/drop_monitor.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)