| Message ID | 20180322225757.10377-2-dsa@cumulusnetworks.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: Allow FIB notifiers to fail add and replace | expand |
On Thu, Mar 22, 2018 at 03:57:51PM -0700, David Ahern wrote: > Notifier handlers use notifier_from_errno to convert any potential error > to an encoded format. As a consequence the other side, call_fib_notifiers > in this case, needs to use notifier_to_errno to return the error from > the handler back to its caller. > > Signed-off-by: David Ahern <dsa@cumulusnetworks.com> > --- > net/core/fib_notifier.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c > index 5ace0705a3f9..14ba52ebe8c9 100644 > --- a/net/core/fib_notifier.c > +++ b/net/core/fib_notifier.c > @@ -21,8 +21,11 @@ EXPORT_SYMBOL(call_fib_notifier); > int call_fib_notifiers(struct net *net, enum fib_event_type event_type, > struct fib_notifier_info *info) There's another (less interesting case) - call_fib_notifier(), which is used to dump the FIB tables for the caller registering to the notification chain. For example, if you have a non-default FIB rule in the system and you modprobe mlxsw, you'll get a silent failure and routes will not be offloaded. On the other hand, I'm not sure we want to fail the module loading in such cases. A possible solution is to have the driver emit a warning via extack for each route/rule being notified after the abort mechanism was triggered. > { > + int err; > + > info->net = net; > - return atomic_notifier_call_chain(&fib_chain, event_type, info); > + err = atomic_notifier_call_chain(&fib_chain, event_type, info); > + return notifier_to_errno(err); > } > EXPORT_SYMBOL(call_fib_notifiers); > > -- > 2.11.0 >
On 3/25/18 2:16 AM, Ido Schimmel wrote: > On Thu, Mar 22, 2018 at 03:57:51PM -0700, David Ahern wrote: >> Notifier handlers use notifier_from_errno to convert any potential error >> to an encoded format. As a consequence the other side, call_fib_notifiers >> in this case, needs to use notifier_to_errno to return the error from >> the handler back to its caller. >> >> Signed-off-by: David Ahern <dsa@cumulusnetworks.com> >> --- >> net/core/fib_notifier.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c >> index 5ace0705a3f9..14ba52ebe8c9 100644 >> --- a/net/core/fib_notifier.c >> +++ b/net/core/fib_notifier.c >> @@ -21,8 +21,11 @@ EXPORT_SYMBOL(call_fib_notifier); >> int call_fib_notifiers(struct net *net, enum fib_event_type event_type, >> struct fib_notifier_info *info) > > There's another (less interesting case) - call_fib_notifier(), which is > used to dump the FIB tables for the caller registering to the > notification chain. > > For example, if you have a non-default FIB rule in the system and you > modprobe mlxsw, you'll get a silent failure and routes will not be > offloaded. On the other hand, I'm not sure we want to fail the module > loading in such cases. right. In normal cases the driver is loaded to create the netdevices long before any networking config is done. So it seems to me the use case you refer to, some user would have go out of there way to create a situation where they install config that is not supported by the driver. > > A possible solution is to have the driver emit a warning via extack for > each route/rule being notified after the abort mechanism was triggered. extack is not available on module load. Per past discussions, something you suggested, we need a message for "out-of-line" cases like this where a driver notifies userspace of a problem.
On Sun, Mar 25, 2018 at 08:00:19AM -0600, David Ahern wrote: > On 3/25/18 2:16 AM, Ido Schimmel wrote: > > On Thu, Mar 22, 2018 at 03:57:51PM -0700, David Ahern wrote: > >> Notifier handlers use notifier_from_errno to convert any potential error > >> to an encoded format. As a consequence the other side, call_fib_notifiers > >> in this case, needs to use notifier_to_errno to return the error from > >> the handler back to its caller. > >> > >> Signed-off-by: David Ahern <dsa@cumulusnetworks.com> > >> --- > >> net/core/fib_notifier.c | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c > >> index 5ace0705a3f9..14ba52ebe8c9 100644 > >> --- a/net/core/fib_notifier.c > >> +++ b/net/core/fib_notifier.c > >> @@ -21,8 +21,11 @@ EXPORT_SYMBOL(call_fib_notifier); > >> int call_fib_notifiers(struct net *net, enum fib_event_type event_type, > >> struct fib_notifier_info *info) > > > > There's another (less interesting case) - call_fib_notifier(), which is > > used to dump the FIB tables for the caller registering to the > > notification chain. > > > > For example, if you have a non-default FIB rule in the system and you > > modprobe mlxsw, you'll get a silent failure and routes will not be > > offloaded. On the other hand, I'm not sure we want to fail the module > > loading in such cases. > > right. In normal cases the driver is loaded to create the netdevices > long before any networking config is done. So it seems to me the use > case you refer to, some user would have go out of there way to create a > situation where they install config that is not supported by the driver. Yes. > > A possible solution is to have the driver emit a warning via extack for > > each route/rule being notified after the abort mechanism was triggered. > > extack is not available on module load. I'm aware. I meant that during module load we'll trigger the abort mechanism (due to an unsupported FIB rule for example), then when user configures additional routes and extack is available we'll emit a warning. > Per past discussions, something you suggested, we need a message for > "out-of-line" cases like this where a driver notifies userspace of a > problem. That's another possibility. We can implement both options to make it perfectly clear to users and daemons what's going on.
diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c index 5ace0705a3f9..14ba52ebe8c9 100644 --- a/net/core/fib_notifier.c +++ b/net/core/fib_notifier.c @@ -21,8 +21,11 @@ EXPORT_SYMBOL(call_fib_notifier); int call_fib_notifiers(struct net *net, enum fib_event_type event_type, struct fib_notifier_info *info) { + int err; + info->net = net; - return atomic_notifier_call_chain(&fib_chain, event_type, info); + err = atomic_notifier_call_chain(&fib_chain, event_type, info); + return notifier_to_errno(err); } EXPORT_SYMBOL(call_fib_notifiers);
Notifier handlers use notifier_from_errno to convert any potential error to an encoded format. As a consequence the other side, call_fib_notifiers in this case, needs to use notifier_to_errno to return the error from the handler back to its caller. Signed-off-by: David Ahern <dsa@cumulusnetworks.com> --- net/core/fib_notifier.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)