Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 16 Jan 2024 08:16:11 -0400 | From | Jason Gunthorpe <> | Subject | Re: [RFC PATCH net-next v5 2/2] net: add netmem to skb_frag_t |
| |
On Tue, Jan 16, 2024 at 07:04:13PM +0800, Yunsheng Lin wrote: > On 2024/1/16 8:01, Jason Gunthorpe wrote: > > On Mon, Jan 15, 2024 at 03:23:33PM -0800, Mina Almasry wrote: > >>>> You did not answer my question that I asked here, and ignoring this > >>>> question is preventing us from making any forward progress on this > >>>> discussion. What do you expect or want skb_frag_page() to do when > >>>> there is no page in the frag? > >>> > >>> I would expect it to do nothing. > >> > >> I don't understand. skb_frag_page() with an empty implementation just > >> results in a compiler error as the function needs to return a page > >> pointer. Do you actually expect skb_frag_page() to unconditionally > >> cast frag->netmem to a page pointer? That was explained as > >> unacceptable over and over again by Jason and Christian as it risks > >> casting devmem to page; completely unacceptable and will get nacked. > >> Do you have a suggestion of what skb_frag_page() should do that will > >> not get nacked by mm? > > > > WARN_ON and return NULL seems reasonable? > > While I am agreed that it may be a nightmare to debug the case of passing > a false page into the mm system, but I am not sure what's the point of > returning NULL to caller if the caller is not expecting or handling > the
You have to return something and NULL will largely reliably crash the thread. The WARN_ON explains in detail why your thread just crashed.
> NULL returning[for example, most of mm API called by the networking does not > seems to handling NULL as input page], isn't the NULL returning will make > the kernel panic anyway? Doesn't it make more sense to just add a BUG_ON() > depending on some configuration like CONFIG_DEBUG_NET or CONFIG_DEVMEM? > As returning NULL seems to be causing a confusion for the caller of > skb_frag_page() as whether to or how to handle the NULL returning case.
Possibly, though Linus doesn't like BUG_ON on principle..
I think the bigger challenge is convincing people that this devmem stuff doesn't just open a bunch of holes in the kernel where userspace can crash it.
The fact you all are debating what to do with skb_frag_page() suggests to me there isn't confidence...
Jason
| ![\](/images/icornerr.gif) |