netdev - Re: [PATCH net-next 2/2] dpaa2-eth: fix return codes used in ndo_setup

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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.