* RIF/VRF overflow in spectrum and reporting errors back to user
@ 2017-10-08 20:10 David Ahern
2017-10-09 9:31 ` Ido Schimmel
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2017-10-08 20:10 UTC (permalink / raw)
To: Ido Schimmel, Jiri Pirko; +Cc: netdev@vger•kernel.org
Jiri / Ido:
I am looking at adding user messages for spectrum failures related to
RIF and VRF overflow coming from the inetaddr and inet6addr notifier
paths. The key is that if the notifiers fail the address add needs to
fail and an error reported to the user as to what happened.
Earlier this year 3ad7d2468f79f added in_validator_info and
in6_validator_info as a way for the notifiers to fail adding an address.
Adding support to spectrum for that notifier is complicated by the fact
that the validator notifier and address notifiers will come in back to
back for the NETDEV_UP case. Ignoring NETDEV_UP in
mlxsw_sp_inetaddr_event seems ok for IPv6 but not clear for IPv4 since
the NETDEV_UP case is emitted on an address delete that involves a
promotion. Handling the back to back NETDEV_UP is complicated since
functions invoked by __mlxsw_sp_inetaddr_event can take multiple
references. Specifically, in mlxsw_sp_port_vlan_router_join():
fid = rif->ops->fid_get(rif);
Can NETDEV_UP be ignored for the inetaddr notifier if it is handled by
the validator notitifer?
David
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: RIF/VRF overflow in spectrum and reporting errors back to user 2017-10-08 20:10 RIF/VRF overflow in spectrum and reporting errors back to user David Ahern @ 2017-10-09 9:31 ` Ido Schimmel 2017-10-10 15:23 ` David Ahern 0 siblings, 1 reply; 4+ messages in thread From: Ido Schimmel @ 2017-10-09 9:31 UTC (permalink / raw) To: David Ahern; +Cc: Ido Schimmel, Jiri Pirko, netdev@vger•kernel.org Hi David, On Sun, Oct 08, 2017 at 02:10:33PM -0600, David Ahern wrote: > Jiri / Ido: > > I am looking at adding user messages for spectrum failures related to > RIF and VRF overflow coming from the inetaddr and inet6addr notifier > paths. The key is that if the notifiers fail the address add needs to > fail and an error reported to the user as to what happened. Thanks for working on this. Very nice idea! > Earlier this year 3ad7d2468f79f added in_validator_info and > in6_validator_info as a way for the notifiers to fail adding an address. > Adding support to spectrum for that notifier is complicated by the fact > that the validator notifier and address notifiers will come in back to > back for the NETDEV_UP case. Ignoring NETDEV_UP in > mlxsw_sp_inetaddr_event seems ok for IPv6 but not clear for IPv4 since > the NETDEV_UP case is emitted on an address delete that involves a > promotion. Handling the back to back NETDEV_UP is complicated since > functions invoked by __mlxsw_sp_inetaddr_event can take multiple > references. Specifically, in mlxsw_sp_port_vlan_router_join(): > fid = rif->ops->fid_get(rif); > > Can NETDEV_UP be ignored for the inetaddr notifier if it is handled by > the validator notitifer? Yes. The case where we get a NETDEV_DOWN for an address delete and then a NETDEV_UP for a promotion is basically a NOP from the driver's perspective. When the NETDEV_DOWN is received, the RIF isn't destroyed because the address list isn't empty (there's an address to be promoted). When the NETDEV_UP is received, it's ignored because we already have a RIF. Regarding IPv6, it's a bit more complicated actually, since we do the actual work in a workqueue, as the notification chain is atomic. I believe this is because the notifier can be called from softirq in response to RA packets. However, this case isn't interesting for mlxsw, as the fact that you process an RA packet suggests you already have a link-local address and thus a RIF. Plus, the kernel won't even process such packets in our case as you most likely have forwarding enabled (unless you tweaked accept_ra for some reason). Looking at ipvlan (the only user of inet6addr_validator_chain), I see that it ignores this specific case and returns NOTIFY_DONE. Maybe we can move this notification chain to be blocking and not call it in response to RA packets seeing that all its users ignore it? Please let me know if you need my help in any way. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: RIF/VRF overflow in spectrum and reporting errors back to user 2017-10-09 9:31 ` Ido Schimmel @ 2017-10-10 15:23 ` David Ahern 2017-10-10 15:47 ` Ido Schimmel 0 siblings, 1 reply; 4+ messages in thread From: David Ahern @ 2017-10-10 15:23 UTC (permalink / raw) To: Ido Schimmel; +Cc: Ido Schimmel, Jiri Pirko, netdev@vger•kernel.org On 10/9/17 3:31 AM, Ido Schimmel wrote: > Hi David, > > On Sun, Oct 08, 2017 at 02:10:33PM -0600, David Ahern wrote: >> Jiri / Ido: >> >> I am looking at adding user messages for spectrum failures related to >> RIF and VRF overflow coming from the inetaddr and inet6addr notifier >> paths. The key is that if the notifiers fail the address add needs to >> fail and an error reported to the user as to what happened. > > Thanks for working on this. Very nice idea! > >> Earlier this year 3ad7d2468f79f added in_validator_info and >> in6_validator_info as a way for the notifiers to fail adding an address. >> Adding support to spectrum for that notifier is complicated by the fact >> that the validator notifier and address notifiers will come in back to >> back for the NETDEV_UP case. Ignoring NETDEV_UP in >> mlxsw_sp_inetaddr_event seems ok for IPv6 but not clear for IPv4 since >> the NETDEV_UP case is emitted on an address delete that involves a >> promotion. Handling the back to back NETDEV_UP is complicated since >> functions invoked by __mlxsw_sp_inetaddr_event can take multiple >> references. Specifically, in mlxsw_sp_port_vlan_router_join(): >> fid = rif->ops->fid_get(rif); >> >> Can NETDEV_UP be ignored for the inetaddr notifier if it is handled by >> the validator notitifer? > > Yes. The case where we get a NETDEV_DOWN for an address delete and then > a NETDEV_UP for a promotion is basically a NOP from the driver's > perspective. When the NETDEV_DOWN is received, the RIF isn't destroyed > because the address list isn't empty (there's an address to be > promoted). When the NETDEV_UP is received, it's ignored because we > already have a RIF. You lost me on the RIF. Looking at the chain: mlxsw_sp_inet6addr_event_work or mlxsw_sp_inetaddr_event - __mlxsw_sp_inetaddr_event + mlxsw_sp_inetaddr_vlan_event * mlxsw_sp_inetaddr_port_vlan_event - NETDEV_UP: mlxsw_sp_port_vlan_router_join mlxsw_sp_port_vlan_router_join does the rif lookup and if it exists calls fid_get() which takes a reference. I read that to mean back-to-back NETDEV_UP notifiers (the address validator and then the address notifier) would lead to a reference count leak. Based on your address delete comment, I take the IPv4 solution to be adding the validator notifier to spectrum and then ignoring NETDEV_UP in mlxsw_sp_inetaddr_event. That means IPv4 inetaddr work is done for the validator notifier while NETDEV_DOWN is done through the inetaddr notifier. > > Regarding IPv6, it's a bit more complicated actually, since we do the > actual work in a workqueue, as the notification chain is atomic. I > believe this is because the notifier can be called from softirq in > response to RA packets. > > However, this case isn't interesting for mlxsw, as the fact that you > process an RA packet suggests you already have a link-local address and > thus a RIF. Plus, the kernel won't even process such packets in our case > as you most likely have forwarding enabled (unless you tweaked accept_ra > for some reason). > > Looking at ipvlan (the only user of inet6addr_validator_chain), I see > that it ignores this specific case and returns NOTIFY_DONE. Maybe we can > move this notification chain to be blocking and not call it in response > to RA packets seeing that all its users ignore it? Seems reasonable to me. I have it coded. Let me test and send an rfc. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: RIF/VRF overflow in spectrum and reporting errors back to user 2017-10-10 15:23 ` David Ahern @ 2017-10-10 15:47 ` Ido Schimmel 0 siblings, 0 replies; 4+ messages in thread From: Ido Schimmel @ 2017-10-10 15:47 UTC (permalink / raw) To: David Ahern; +Cc: Ido Schimmel, Jiri Pirko, netdev@vger•kernel.org On Tue, Oct 10, 2017 at 09:23:48AM -0600, David Ahern wrote: > On 10/9/17 3:31 AM, Ido Schimmel wrote: > >> Can NETDEV_UP be ignored for the inetaddr notifier if it is handled by > >> the validator notitifer? > > > > Yes. The case where we get a NETDEV_DOWN for an address delete and then > > a NETDEV_UP for a promotion is basically a NOP from the driver's > > perspective. When the NETDEV_DOWN is received, the RIF isn't destroyed > > because the address list isn't empty (there's an address to be > > promoted). When the NETDEV_UP is received, it's ignored because we > > already have a RIF. > > You lost me on the RIF. Looking at the chain: > > mlxsw_sp_inet6addr_event_work or mlxsw_sp_inetaddr_event > - __mlxsw_sp_inetaddr_event > + mlxsw_sp_inetaddr_vlan_event > * mlxsw_sp_inetaddr_port_vlan_event > - NETDEV_UP: mlxsw_sp_port_vlan_router_join > > mlxsw_sp_port_vlan_router_join does the rif lookup and if it exists > calls fid_get() which takes a reference. I read that to mean > back-to-back NETDEV_UP notifiers (the address validator and then the > address notifier) would lead to a reference count leak. > > Based on your address delete comment, I take the IPv4 solution to be > adding the validator notifier to spectrum and then ignoring NETDEV_UP in > mlxsw_sp_inetaddr_event. That means IPv4 inetaddr work is done for the > validator notifier while NETDEV_DOWN is done through the inetaddr notifier. Exactly. The only NETDEV_UP we "miss" is the one sent for the promoted address in the inetaddr chain, but it's irrelevant because when we got the preceding NETDEV_DOWN for the deleted primary address we didn't destroy the RIF as the address list wasn't empty (see mlxsw_sp_rif_should_config() which is called by both top functions in your call chain). > > Regarding IPv6, it's a bit more complicated actually, since we do the > > actual work in a workqueue, as the notification chain is atomic. I > > believe this is because the notifier can be called from softirq in > > response to RA packets. > > > > However, this case isn't interesting for mlxsw, as the fact that you > > process an RA packet suggests you already have a link-local address and > > thus a RIF. Plus, the kernel won't even process such packets in our case > > as you most likely have forwarding enabled (unless you tweaked accept_ra > > for some reason). > > > > Looking at ipvlan (the only user of inet6addr_validator_chain), I see > > that it ignores this specific case and returns NOTIFY_DONE. Maybe we can > > move this notification chain to be blocking and not call it in response > > to RA packets seeing that all its users ignore it? > > Seems reasonable to me. > > I have it coded. Let me test and send an rfc. Great. Looking forward to it. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-10 15:47 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-08 20:10 RIF/VRF overflow in spectrum and reporting errors back to user David Ahern 2017-10-09 9:31 ` Ido Schimmel 2017-10-10 15:23 ` David Ahern 2017-10-10 15:47 ` Ido Schimmel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox