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 linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 24 Apr 2020 09:04:26 +0200 From: Jesper Dangaard Brouer <brouer@...hat.com> To: Jakub Kicinski <kuba@...nel.org> Cc: 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, brouer@...hat.com Subject: Re: [PATCH net-next 2/2] dpaa2-eth: fix return codes used in ndo_setup_tc On Thu, 23 Apr 2020 12:56:00 -0700 Jakub Kicinski <kuba@...nel.org> wrote: > On Thu, 23 Apr 2020 12:33:56 -0700 Stephen Hemminger wrote: > > On Thu, 23 Apr 2020 17:38:04 +0200 > > Jesper Dangaard Brouer <brouer@...hat.com> wrote: > > > > > On Thu, 23 Apr 2020 08:28:58 -0700 > > > Stephen Hemminger <stephen@...workplumber.org> wrote: > > > > > > > On Thu, 23 Apr 2020 16:57:50 +0200 > > > > Jesper Dangaard Brouer <brouer@...hat.com> wrote: > > > > > > > > > Drivers ndo_setup_tc call should return -EOPNOTSUPP, when it cannot > > > > > support the qdisc type. Other return values will result in failing the > > > > > qdisc setup. This lead to qdisc noop getting assigned, which will > > > > > drop all TX packets on the interface. > > > > > > > > > > Fixes: ab1e6de2bd49 ("dpaa2-eth: Add mqprio support") > > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com> > > > > > > > > Would it be possible to use extack as well? > > > > > > That is what patch 1/2 already does. > > > > > > > Putting errors in dmesg is unhelpful > > > > > > This patchset does not introduce any dmesg printk. > > > > > > > I was thinking that this > > if (num_tc > dpaa2_eth_tc_count(priv)) { > > netdev_err(net_dev, "Max %d traffic classes supported\n", > > dpaa2_eth_tc_count(priv)); > > - return -EINVAL; > > + return -EOPNOTSUPP; > > } > > > > could be an extack message First of all, this is a fix, and we need to keep it simple, as it needs to be backported to v5.3. Talking about converting this warning message this into a extack, I'm actually not convinced that is a good idea, or will even work. First the extack cannot contain the %d number. Second returning -EOPNOTSUPP this is actually not an error, and I don't think tc will print the extack in that case? > That's a good question, actually. In this case Jesper was seeing a > failure when creating the default qdisc. The extack would go nowhere, > we'd have to print it to the logs, no? Which we should probably do, > anyway. Good point. We probably need a separate dmesg error when we cannot configure the default qdisc. As there is not end-user to receive the extack. But I would place that at a higher level in qdisc_create_dflt(). It would definitely have helped me to identify what net-subsystem was dropping packets, and after my patch[1/2] adding the extack, an end-user would get a meaning full message to ease the troubleshooting. (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.) (For a separate patch:) We should discuss, that when creating the default qdisc, we should IMHO not allow that to fail. As you can see in [1], this step happens during the qdisc init function e.g. it could also fail due to low memory. IMHO we should have a fallback, for when the default qdisc init fails, e.g. assign pfifo_fast instead or even noqueue. > > 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. > 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? [1] https://github.com/xdp-project/xdp-project/blob/master/areas/arm64/board_nxp_ls1088/nxp-board04-troubleshoot-qdisc.org -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Powered by blists - more mailing lists