lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 27 Apr 2020 11:18:43 -0300 From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com> To: Jakub Kicinski <kuba@...nel.org> Cc: Jesper Dangaard Brouer <brouer@...hat.com>, Stephen Hemminger <stephen@...workplumber.org>, netdev@...r.kernel.org, Ilias Apalodimas <ilias.apalodimas@...aro.org>, Toke Høiland-Jørgensen <toke@...hat.com>, ruxandra.radulescu@....com, ioana.ciornei@....com, nipun.gupta@....com, shawnguo@...nel.org Subject: Re: [PATCH net-next 2/2] dpaa2-eth: fix return codes used in ndo_setup_tc On Fri, Apr 24, 2020 at 05:28:59PM -0700, Jakub Kicinski wrote: > On Fri, 24 Apr 2020 09:04:26 +0200 Jesper Dangaard Brouer wrote: > > (Side-note: First I placed an extack in qdisc_create_dflt() but I > > realized it was wrong, because it could potentially override messages > > from the lower layers.) > > > > > > but doing that would require a change > > > > to the ndo_setup_tc hook to allow driver to return its own error message > > > > as to why the setup failed. > > > > > > Yeah :S The block offload command contains extack, but this driver > > > doesn't understand block offload, so it won't interpret it... > > > > > > That brings me to an important point - doesn't the extack in patch 1 > > > override any extack driver may have set? > > > > Nope, see above side-note. I set the extack at the "lowest level", > > e.g. closest to the error that cause the err back-propagation, when I > > detect that this will cause a failure at higher level. > > Still, the driver is lower: > > diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c > index 2908e0a0d6e1..ffed75453c14 100644 > --- a/drivers/net/netdevsim/netdev.c > +++ b/drivers/net/netdevsim/netdev.c > @@ -209,9 +209,12 @@ static int > nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data) > { > struct netdevsim *ns = netdev_priv(dev); > + struct flow_block_offload *f; > > switch (type) { > case TC_SETUP_BLOCK: > + f = type_data; > + NL_SET_ERR_MSG_MOD(f->extack, "bla bla bla bla bla"); > + return -EINVAL; > - return flow_block_cb_setup_simple(type_data, > - &nsim_block_cb_list, > - nsim_setup_tc_block_cb, > default: > return -EOPNOTSUPP; > } > > # tc qdisc add dev netdevsim0 ingress > Error: Driver ndo_setup_tc failed. > > > > > I remember we discussed this when adding extacks to the TC core, but > > > I don't remember the conclusion now, ugh. > > > > When adding the extack code, I as puzzled that during debugging I > > managed to override other extack messages. Have anyone though about a > > better way to handle if extack messages gets overridden? > > I think there was more discussion, but this is all I can find now: > > https://lore.kernel.org/netdev/20180918131212.20266-4-johannes@sipsolutions.net/#t > > Maybe Marcelo will remeber. There was also this other one, on supporting multiple messages: https://lore.kernel.org/netdev/673abaddb26351826ca454f46d1271f1f4814c56.1521226621.git.marcelo.leitner%40gmail.com/T/ What I remember is what we have now is enough because of 3 main reasons: - Anything logged before the actual error is potentially just noise - Anything logged after the actual error is just noise, - The user is probably not prepared to handle warnings from ip/tc/etc commands. For this last, one example using the context here: "Warning: failed to create the qdisc." Ok, but what does that mean? Should the sysadmin, potentially unaware of what a qdisc is, retry or ignore the warning? If retry, then it probably should have just failed itself in the first place.
Powered by blists - more mailing lists