[PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[Julien Gomes]

When sending a cache report on mroute_sk, mroute will emit a
"pending queue full" warning for every error value returned by
sock_queue_rcv_skb().
This warning can be misleading, for example on the EPERM error value
that sk_filter() can return.

Restricting this warning to only ENOMEM or ENOBUFS seems more
appropriate.

Signed-off-by: Julien Gomes <julien@arista.com>
---
 net/ipv4/ipmr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index a1d521be612b..ace12cddb9de 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1068,7 +1068,8 @@ static int ipmr_cache_report(struct mr_table *mrt,
 	ret = sock_queue_rcv_skb(mroute_sk, skb);
 	rcu_read_unlock();
 	if (ret < 0) {
-		net_warn_ratelimited("mroute: pending queue full, dropping entries\n");
+		if (ret == -ENOMEM || ret == -ENOBUFS)
+			net_warn_ratelimited("mroute: pending queue full, dropping entries\n");
 		kfree_skb(skb);
 	}
 
-- 
2.13.1

[PATCH net-next 2/2] ip6mr: restrict mroute6 "queue full" warning to related error values

[Julien Gomes]

When sending a cache report on mroute6_sk, mroute6 will emit a
"pending queue full" warning for every error value returned by
sock_queue_rcv_skb().
This warning can be misleading, for example on the EPERM error value
that sk_filter() can return.

Restricting this warning to only ENOMEM or ENOBUFS seems more
appropriate.

Signed-off-by: Julien Gomes <julien@arista.com>
---
 net/ipv6/ip6mr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 7454850f2098..62fe5fd64f22 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -1215,7 +1215,8 @@ static int ip6mr_cache_report(struct mr6_table *mrt, struct sk_buff *pkt,
 	 */
 	ret = sock_queue_rcv_skb(mrt->mroute6_sk, skb);
 	if (ret < 0) {
-		net_warn_ratelimited("mroute6: pending queue full, dropping entries\n");
+		if (ret == -ENOMEM || ret == -ENOBUFS)
+			net_warn_ratelimited("mroute6: pending queue full, dropping entries\n");
 		kfree_skb(skb);
 	}
 
-- 
2.13.1

Re: [PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[David Miller]

From: Julien Gomes <julien@arista.com>
Date: Wed, 21 Jun 2017 10:58:10 -0700

> When sending a cache report on mroute_sk, mroute will emit a
> "pending queue full" warning for every error value returned by
> sock_queue_rcv_skb().
> This warning can be misleading, for example on the EPERM error value
> that sk_filter() can return.
> 
> Restricting this warning to only ENOMEM or ENOBUFS seems more
> appropriate.
> 
> Signed-off-by: Julien Gomes <julien@arista.com>

Incorrect, no other error codes are possible.

We never attach a socket filter to these kernel internal sockets,
therefore sk_filter() is not even applicable in this analysis.

Therefore, -ENOBUFS and -ENOMEM are the only errors we can ever see
returned from sock_queue_rcv_skb().

This goes for your second patch as well.

Re: [PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[Julien Gomes]

On 06/23/2017 10:39 AM, David Miller wrote:

> From: Julien Gomes <julien@arista.com>
> Date: Wed, 21 Jun 2017 10:58:10 -0700
>
>> When sending a cache report on mroute_sk, mroute will emit a
>> "pending queue full" warning for every error value returned by
>> sock_queue_rcv_skb().
>> This warning can be misleading, for example on the EPERM error value
>> that sk_filter() can return.
>>
>> Restricting this warning to only ENOMEM or ENOBUFS seems more
>> appropriate.
>>
>> Signed-off-by: Julien Gomes <julien@arista.com>
> Incorrect, no other error codes are possible.
>
> We never attach a socket filter to these kernel internal sockets,
> therefore sk_filter() is not even applicable in this analysis.
>
> Therefore, -ENOBUFS and -ENOMEM are the only errors we can ever see
> returned from sock_queue_rcv_skb().
>
> This goes for your second patch as well.

Up to now I would agree, but now that cache reports are also sent
through Netlink, wouldn't it make sense to allow the user of mroute_sk
to attach a filter to it in order to not receive cache reports on it?

I agree this is not crucial in any way, but this could be a way to let
the user program choose whether it receives the reports through one
of the mediums, and not inevitably both.

-- 
Julien Gomes

Re: [PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[David Miller]

From: Julien Gomes <julien@arista.com>
Date: Fri, 23 Jun 2017 10:52:26 -0700

> On 06/23/2017 10:39 AM, David Miller wrote:
> 
>> From: Julien Gomes <julien@arista.com>
>> Date: Wed, 21 Jun 2017 10:58:10 -0700
>>
>>> When sending a cache report on mroute_sk, mroute will emit a
>>> "pending queue full" warning for every error value returned by
>>> sock_queue_rcv_skb().
>>> This warning can be misleading, for example on the EPERM error value
>>> that sk_filter() can return.
>>>
>>> Restricting this warning to only ENOMEM or ENOBUFS seems more
>>> appropriate.
>>>
>>> Signed-off-by: Julien Gomes <julien@arista.com>
>> Incorrect, no other error codes are possible.
>>
>> We never attach a socket filter to these kernel internal sockets,
>> therefore sk_filter() is not even applicable in this analysis.
>>
>> Therefore, -ENOBUFS and -ENOMEM are the only errors we can ever see
>> returned from sock_queue_rcv_skb().
>>
>> This goes for your second patch as well.
> 
> Up to now I would agree, but now that cache reports are also sent
> through Netlink, wouldn't it make sense to allow the user of mroute_sk
> to attach a filter to it in order to not receive cache reports on it?

There is not visibility of this socket outside of the kernel.

I doubt it would ever be exported in any way, and until it would
be so worrying about this is truly a huge waste of time and developer
resources.

Thank you.

Re: [PATCH net-next 1/2] ipmr: restrict mroute "queue full" warning to related error values

[Julien Gomes]

On 06/23/2017 11:47 AM, David Miller wrote:

> From: Julien Gomes <julien@arista.com>
> Date: Fri, 23 Jun 2017 10:52:26 -0700
>
>> On 06/23/2017 10:39 AM, David Miller wrote:
>>
>>> From: Julien Gomes <julien@arista.com>
>>> Date: Wed, 21 Jun 2017 10:58:10 -0700
>>>
>>>> When sending a cache report on mroute_sk, mroute will emit a
>>>> "pending queue full" warning for every error value returned by
>>>> sock_queue_rcv_skb().
>>>> This warning can be misleading, for example on the EPERM error value
>>>> that sk_filter() can return.
>>>>
>>>> Restricting this warning to only ENOMEM or ENOBUFS seems more
>>>> appropriate.
>>>>
>>>> Signed-off-by: Julien Gomes <julien@arista.com>
>>> Incorrect, no other error codes are possible.
>>>
>>> We never attach a socket filter to these kernel internal sockets,
>>> therefore sk_filter() is not even applicable in this analysis.
>>>
>>> Therefore, -ENOBUFS and -ENOMEM are the only errors we can ever see
>>> returned from sock_queue_rcv_skb().
>>>
>>> This goes for your second patch as well.
>> Up to now I would agree, but now that cache reports are also sent
>> through Netlink, wouldn't it make sense to allow the user of mroute_sk
>> to attach a filter to it in order to not receive cache reports on it?
> There is not visibility of this socket outside of the kernel.

Hmm, maybe we are not talking about the same thing.

The value of this socket pointer is set by the MRT_INIT sockopt.
The socket is definitely visible outside of the kernel as it is first
created and used by the user for kernel-user communications
via the sockopts and the messages sent by the kernel to the user
through it.


The typical user setup up to now was to create this socket,
MRT_INIT to register it in ipmr, and handle the incoming packets,
including the cache reports.

Now that the cache reports are also sent through another medium,
the user should be able to decide whether it also wants the
reports on this socket, which, once again, it is already using.
And if the user wants to get the reports only through Netlink,
the kernel will currently emit those unrelated warnings.

Once again, we are not in the case of a purely internal kernel socket,
as this socket is used for internal kernel-user communications.

-- 
Julien Gomes