netdev - Re: [PATCH v2 net-next 3/6] sfc: optional logging of TC offload errors

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

Hash Suite for Android: free password hash cracker in your pocket

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

Date:   Wed, 28 Sep 2022 12:07:54 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Edward Cree <ecree.xilinx@...il.com>
Cc:     netdev@...r.kernel.org, linux-net-drivers@....com,
        davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
        habetsm.xilinx@...il.com
Subject: Re: [PATCH v2 net-next 3/6] sfc: optional logging of TC offload
 errors

On Wed, 28 Sep 2022 19:58:23 +0100 Edward Cree wrote:
> On 28/09/2022 19:32, Jakub Kicinski wrote:
> > I won't help with the indirect stuff, I fixed it once a while
> > back already and it keeps getting broken. It must be a case of 
> > the extack not being plumbed thru, or people being conservative
> > because the errors are not fatal, right? Solvable.  
> 
> The conceptual problem, as I see it, is that multiple hw drivers /
>  driver instances might be trying to offload the same tunnel rule,
>  because the ingress device isn't actually specified anywhere in
>  the weird inside-out way TC tunnel rules work.
> So how do you deal with the case where one driver succeeds and
>  another fails to offload, or two fail with different rc and
>  extack messages?

Let's solve practical problems first :) The cases with multiple devices
offloading are rare AFAIK.

> But I really need to go and check what it does right now, because
>  my information might be out of date — some of this driver code
>  was first written two years ago so maybe it's since been solved.
> 
> > The printf'ing? I recon something simple like adding a destructor 
> > for the message to the exack struct so you can allocate the message,   
> 
> What about just a flag to say "please kfree() the msg on destruct"?
> I have a hard time imagining a destructor that would need to do
>  anything different.

Yes, seems like that could be good enough. I was wondering if perhaps
someone would like to have a "static" buffer and manage ownership (given
most of the config happens under rtnl_lock) but perhaps that's
unnecessary complexity.

BTW we will prolly need two bits, one to indicate the creator will
actually call free and the other to mark that it's needed. Otherwise
we'd need to sift thru the stack and find all extack instances.
You can if you want to but...

> > or adding a small buffer in place (the messages aren't very long,
> > usually) come to mind.  
> 
> Also an option, yeah.  Downside is that it consumes that memory
>  (I guess 80B or so?) for every netlink response whether it's using
>  formatted extack or not; idk if people with lots of netlink
>  traffic might start to care about that.

It's just a buffer on the stack, in the struct, the extack is
transformed into netlink attrs in the same way regardless.
Stack use is the only concern, no other impact on those not using it.

> Should I rustle up an RFC patch for one of these, or post an RFD to
>  the list to canvass other vendors' opinions?

Would be great! Maybe also grep the archive, cause this came up before.
Someone was against this in the past, perhaps, perhaps even me :)
But if it wasn't me we should CC them.