[PATCH] drop_monitor: consider inserted data in genlmsg_end

[PATCH] drop_monitor: consider inserted data in genlmsg_end

[Reiter Wolfgang]

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

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[Neil Horman]

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>

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[David Miller]

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.

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[Neil Horman]

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?

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[David Miller]

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.

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[Wolfgang Reiter]

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?

Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end

[David Miller]

From: Reiter Wolfgang <wr0112358@gmail.com>
Date: Tue,  3 Jan 2017 00:34: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>

Several coding style errors:

> @@ -112,6 +111,12 @@ 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) {

There must be a space between "if" and "(skb)"

> +		struct nlmsghdr *nlh = (struct nlmsghdr *)skb->data;
> +		struct genlmsghdr *gnlh = (struct genlmsghdr *)nlmsg_data(nlh);
> +		genlmsg_end(skb, genlmsg_data(gnlh));
> +	}

There should be an empty line between the local variable declarations
and actual code.

[PATCH] drop_monitor: consider inserted data in genlmsg_end

[Reiter Wolfgang]

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index f465bad..ccaaf3e 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,12 @@ 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