Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549469 > unrolled thread

[PATCH] drop_monitor: consider inserted data in genlmsg_end

Back to article view | Back to linux.kernel

Contents

  [PATCH] drop_monitor: consider inserted data in genlmsg_end Reiter Wolfgang <wr0112358@gmail.com> - 2017-01-03 01:40 +0100
    Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end Neil Horman <nhorman@tuxdriver.com> - 2017-01-03 14:20 +0100
    Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end David Miller <davem@davemloft.net> - 2017-01-03 16:00 +0100
      Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end Neil Horman <nhorman@tuxdriver.com> - 2017-01-03 17:10 +0100
        Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end David Miller <davem@davemloft.net> - 2017-01-03 17:20 +0100
        Re: [PATCH] drop_monitor: consider inserted data in genlmsg_end Wolfgang Reiter <wr0112358@gmail.com> - 2017-01-04 00:20 +0100

#1549469 — [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 <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

[toc] | [next] | [standalone]

#1549746

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>

[toc] | [prev] | [next] | [standalone]

#1549839

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.

[toc] | [prev] | [next] | [standalone]

#1549915

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?

[toc] | [prev] | [next] | [standalone]

#1549921

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.

[toc] | [prev] | [next] | [standalone]

#1550280

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?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web