| Message ID | 20170621175811.16940-1-julien@arista.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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.
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.
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); }
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(-)