[PATCH net] netlink: fix netlink_ack with large messages

[PATCH net] netlink: fix netlink_ack with large messages

[Jiri Benc]

Commit c05cdb1b864f ("netlink: allow large data transfers from user-space")
does not handle cases where netlink_ack is used to report an error. In such
case, the original message is copied to the ack message, which needs to be
large enough.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/netlink/af_netlink.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 8df7f64..090f624 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1809,7 +1809,7 @@ out_put:
 	sock_put(sk);
 out:
 #endif
-	return alloc_skb(size, gfp_mask);
+	return netlink_alloc_large_skb(size, gfp_mask);
 }
 EXPORT_SYMBOL_GPL(netlink_alloc_skb);
 
-- 
1.7.6.5

Re: [PATCH net] netlink: fix netlink_ack with large messages

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Thu,  7 Nov 2013 19:57:45 +0100

> Commit c05cdb1b864f ("netlink: allow large data transfers from user-space")
> does not handle cases where netlink_ack is used to report an error. In such
> case, the original message is copied to the ack message, which needs to be
> large enough.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>

I have two problems with this change.

First of all, if netlink_ack() has this problem, do not extend the
netlink_alloc_large_skb() usage to dumps too as your patch is
doing here.

Secondly, it seems sort of over the top to quote such enormous
messages, and in fact wasteful.  We have the sequence number in
the netlink header, so the user can tell exactly which message
we are erroring.

Just quoting such huge requests in ACKs by default doesn't seem
to make any sense.  I would say that we should have a way to
turn off the quoting, or at least limit it, and turn this knob
off for things like nftables that can hit these kinds of cases.

Pablo, what do you think?

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Thomas Graf]

On 11/08/13 at 03:07pm, David Miller wrote:
> From: Jiri Benc <jbenc@redhat.com>
> Date: Thu,  7 Nov 2013 19:57:45 +0100
> 
> > Commit c05cdb1b864f ("netlink: allow large data transfers from user-space")
> > does not handle cases where netlink_ack is used to report an error. In such
> > case, the original message is copied to the ack message, which needs to be
> > large enough.
> > 
> > Signed-off-by: Jiri Benc <jbenc@redhat.com>
> 
> I have two problems with this change.
> 
> First of all, if netlink_ack() has this problem, do not extend the
> netlink_alloc_large_skb() usage to dumps too as your patch is
> doing here.
> 
> Secondly, it seems sort of over the top to quote such enormous
> messages, and in fact wasteful.  We have the sequence number in
> the netlink header, so the user can tell exactly which message
> we are erroring.
> 
> Just quoting such huge requests in ACKs by default doesn't seem
> to make any sense.  I would say that we should have a way to
> turn off the quoting, or at least limit it, and turn this knob
> off for things like nftables that can hit these kinds of cases.

I agree it seems over the top for pure ACKs but we also use
netlink_ack() to report errors where it makes sense to quote
the full message.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Sat, 9 Nov 2013 00:04:34 +0000

> I agree it seems over the top for pure ACKs but we also use
> netlink_ack() to report errors where it makes sense to quote
> the full message.

The user has the message, they gave it to us in the sendmsg()
we are responding to.  We absolutely do not need to give it
to them again.

If they care about referring to the contents of that message, they can
refer to it in their own copy and make sure they are really looking at
the same thing by comparing the sequence number in the netlink ACK to
the one they used in the netlink header they gave to the kernel in the
sendmsg() call.

What happens now is pure duplication, and for such huge netlink
messages it's really not smart at all.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Jamal Hadi Salim]

On 11/09/13 00:00, David Miller wrote:

> The user has the message, they gave it to us in the sendmsg()
> we are responding to.  We absolutely do not need to give it
> to them again.
>
> If they care about referring to the contents of that message, they can
> refer to it in their own copy and make sure they are really looking at
> the same thing by comparing the sequence number in the netlink ACK to
> the one they used in the netlink header they gave to the kernel in the
> sendmsg() call.
>
> What happens now is pure duplication, and for such huge netlink
> messages it's really not smart at all.
>

for errors, we need to give the user something back. This has been the
behavior for 80 years now. Giving them a HUGE message
back is rediculuos(tm). Ive had enough of SCTP doing that.
We need to cap it - sort of what ICMP does.
ICMP caps at 64B; something like 128B is reasonable.

cheers,
jamal

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Pablo Neira Ayuso]

On Sat, Nov 09, 2013 at 08:43:51AM -0500, Jamal Hadi Salim wrote:
> On 11/09/13 00:00, David Miller wrote:
> 
> >The user has the message, they gave it to us in the sendmsg()
> >we are responding to.  We absolutely do not need to give it
> >to them again.
> >
> >If they care about referring to the contents of that message, they can
> >refer to it in their own copy and make sure they are really looking at
> >the same thing by comparing the sequence number in the netlink ACK to
> >the one they used in the netlink header they gave to the kernel in the
> >sendmsg() call.
> >
> >What happens now is pure duplication, and for such huge netlink
> >messages it's really not smart at all.
> >
> 
> for errors, we need to give the user something back. This has been the
> behavior for 80 years now. Giving them a HUGE message
> back is rediculuos(tm). Ive had enough of SCTP doing that.
> We need to cap it - sort of what ICMP does.
> ICMP caps at 64B; something like 128B is reasonable.

Personally, I have only used the sequence number to correlate the
original request with the ack reply, so I agree in that trimming it to
some reasonable amount of bytes like ICMP is the way to go. I prefer
if we select a large enough amount of bytes to avoid breaking backward
compatibility, eg. 128KB, since I'm not sure what kind of handling
others may have done of this.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Jiri Benc]

On Sat, 9 Nov 2013 19:03:53 +0100, Pablo Neira Ayuso wrote:
> On Sat, Nov 09, 2013 at 08:43:51AM -0500, Jamal Hadi Salim wrote:
> > for errors, we need to give the user something back. This has been the
> > behavior for 80 years now. Giving them a HUGE message
> > back is rediculuos(tm). Ive had enough of SCTP doing that.
> > We need to cap it - sort of what ICMP does.
> > ICMP caps at 64B; something like 128B is reasonable.
> 
> Personally, I have only used the sequence number to correlate the
> original request with the ack reply, so I agree in that trimming it to
> some reasonable amount of bytes like ICMP is the way to go. I prefer
> if we select a large enough amount of bytes to avoid breaking backward
> compatibility, eg. 128KB, since I'm not sure what kind of handling
> others may have done of this.

Do you think capping at NLMSG_GOODSIZE would be too low? The allocation
won't fit into one page with NLMSG_GOODSIZE but I doubt we can go lower
than that. Alternatively, we can do some math to fully use the two
pages, like
	NLMSG_GOODSIZE + min(PAGE_SIZE, 8192UL) - NLMSG_HDRLEN - NLMSG_ALIGN(sizeof(struct nlmsgerr))
(which I'm not sure is worth it).

 Jiri

-- 
Jiri Benc

Re: [PATCH net] netlink: fix netlink_ack with large messages

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Tue, 12 Nov 2013 16:29:07 +0100

> On Sat, 9 Nov 2013 19:03:53 +0100, Pablo Neira Ayuso wrote:
>> On Sat, Nov 09, 2013 at 08:43:51AM -0500, Jamal Hadi Salim wrote:
>> > for errors, we need to give the user something back. This has been the
>> > behavior for 80 years now. Giving them a HUGE message
>> > back is rediculuos(tm). Ive had enough of SCTP doing that.
>> > We need to cap it - sort of what ICMP does.
>> > ICMP caps at 64B; something like 128B is reasonable.
>> 
>> Personally, I have only used the sequence number to correlate the
>> original request with the ack reply, so I agree in that trimming it to
>> some reasonable amount of bytes like ICMP is the way to go. I prefer
>> if we select a large enough amount of bytes to avoid breaking backward
>> compatibility, eg. 128KB, since I'm not sure what kind of handling
>> others may have done of this.
> 
> Do you think capping at NLMSG_GOODSIZE would be too low? The allocation
> won't fit into one page with NLMSG_GOODSIZE but I doubt we can go lower
> than that. Alternatively, we can do some math to fully use the two
> pages, like
> 	NLMSG_GOODSIZE + min(PAGE_SIZE, 8192UL) - NLMSG_HDRLEN - NLMSG_ALIGN(sizeof(struct nlmsgerr))
> (which I'm not sure is worth it).

I don't think this is the way to go.

I think since existing apps expect the whole message, we have to
provide it.

We should add a new socket option so that applications can ask that
messages not be quoted in ACKs, as I've stated a few times already in
this thread.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Jiri Benc]

On Tue, 12 Nov 2013 14:35:15 -0500 (EST), David Miller wrote:
> > Do you think capping at NLMSG_GOODSIZE would be too low? The allocation
> > won't fit into one page with NLMSG_GOODSIZE but I doubt we can go lower
> > than that. Alternatively, we can do some math to fully use the two
> > pages, like
> > 	NLMSG_GOODSIZE + min(PAGE_SIZE, 8192UL) - NLMSG_HDRLEN - NLMSG_ALIGN(sizeof(struct nlmsgerr))
> > (which I'm not sure is worth it).
> 
> I don't think this is the way to go.
> 
> I think since existing apps expect the whole message, we have to
> provide it.
> 
> We should add a new socket option so that applications can ask that
> messages not be quoted in ACKs, as I've stated a few times already in
> this thread.

I completely agree with this, sorry for not being clear. I just
understood from the thread that the way to go is to do both, in order
to not generate too large ACKs for the _new_ code (i.e. for the
messages that were not plausible before "netlink: allow large data
transfers from user-space"). I don't know what the "too large" should
be, though, hence the question.

But then, if we don't do any capping, the only outcome of a failed
allocation is the ACK won't be sent and it's clearly stated that
netlink does not provide reliability. Works for me.

 Jiri

-- 
Jiri Benc

Re: [PATCH net] netlink: fix netlink_ack with large messages

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Wed, 13 Nov 2013 12:25:04 +0100

> I completely agree with this, sorry for not being clear. I just
> understood from the thread that the way to go is to do both, in order
> to not generate too large ACKs for the _new_ code (i.e. for the
> messages that were not plausible before "netlink: allow large data
> transfers from user-space"). I don't know what the "too large" should
> be, though, hence the question.
> 
> But then, if we don't do any capping, the only outcome of a failed
> allocation is the ACK won't be sent and it's clearly stated that
> netlink does not provide reliability. Works for me.

Of course, we should meanwhile add the large SKB handling to the
netlink ACK code.

Therefore, please resubmit your original patch, but modify it as
I asked such that only the ACK code path gets the new large SKB
call.

Thanks.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Sat, 09 Nov 2013 08:43:51 -0500

> for errors, we need to give the user something back. This has been the
> behavior for 80 years now. Giving them a HUGE message
> back is rediculuos(tm). Ive had enough of SCTP doing that.
> We need to cap it - sort of what ICMP does.
> ICMP caps at 64B; something like 128B is reasonable.

It is correct that we really can't change existing behavior.

I want to do something smarter in the new cases where we can.

nftables is the first thing that works with such enormous
messages, so let's create a facility such that nftables
netlink users don't need to get the entire quote message
back.

That's why I suggested a per-subsystem flag, that entities like
nftables can set when it registers, that says "don't quote the message
in the ACK."

Or, alternatively, let's have the application set this flag,
via a socket option or similar.

Both approaches work for me, and the latter probably gains us
the most over time as we can make sure that eventually all the
major netlink apps start setting the flag.

Re: [PATCH net] netlink: fix netlink_ack with large messages

[Pablo Neira Ayuso]

On Sat, Nov 09, 2013 at 02:27:06PM -0500, David Miller wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Date: Sat, 09 Nov 2013 08:43:51 -0500
> 
> > for errors, we need to give the user something back. This has been the
> > behavior for 80 years now. Giving them a HUGE message
> > back is rediculuos(tm). Ive had enough of SCTP doing that.
> > We need to cap it - sort of what ICMP does.
> > ICMP caps at 64B; something like 128B is reasonable.
> 
> It is correct that we really can't change existing behavior.
> 
> I want to do something smarter in the new cases where we can.
> 
> nftables is the first thing that works with such enormous
> messages, so let's create a facility such that nftables
> netlink users don't need to get the entire quote message
> back.
>
> That's why I suggested a per-subsystem flag, that entities like
> nftables can set when it registers, that says "don't quote the message
> in the ACK."
> 
> Or, alternatively, let's have the application set this flag,
> via a socket option or similar.
> 
> Both approaches work for me, and the latter probably gains us
> the most over time as we can make sure that eventually all the
> major netlink apps start setting the flag.

In the nftables case, we send a large packet containing small netlink
messages, so it's unlikely that we'll hit the problem that Jiri
reported since the ack is reported back per small message in the
packet.

But we still have to fix this for other netlink subsystems following
either approach, David's flag or Jamal's netlink error with origin
netlink message cap.