[PATCH] drop_monitor: consider inserted data in genlmsg_end

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 <redacted>
---
 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

On Tue, Jan 03, 2017 at 01:39:10AM +0100, Reiter Wolfgang wrote:

Acked-by: Neil Horman <nhorman@tuxdriver.com>

From: Reiter Wolfgang <redacted>
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 <redacted>

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 <redacted>
Date: Tue,  3 Jan 2017 01:39:10 +0100

quoted

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 <redacted>

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:

quoted

From: Reiter Wolfgang <redacted>
Date: Tue,  3 Jan 2017 01:39:10 +0100

quoted

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 <redacted>

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 [off-list ref] writes:

On Tue, Jan 03, 2017 at 09:54:19AM -0500, David Miller wrote:

quoted

From: Reiter Wolfgang <redacted>
Date: Tue,  3 Jan 2017 01:39:10 +0100

quoted

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 <redacted>

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?