On Wed, Jan 03, 2024 at 02:53:20PM -0800, Martin KaFai Lau wrote:
> On 1/3/24 4:04 AM, Maciej Fijalkowski wrote:
> > On Tue, Jan 02, 2024 at 02:58:00PM -0800, Martin KaFai Lau wrote:
> > > On 12/21/23 5:26 AM, Maciej Fijalkowski wrote:
> > > > This comes from __xdp_return() call with xdp_buff argument passed as
> > > > NULL which is supposed to be consumed by xsk_buff_free() call.
> > > >
> > > > To address this properly, in ZC case, a node that represents the frag
> > > > being removed has to be pulled out of xskb_list. Introduce
> > > > appriopriate xsk helpers to do such node operation and use them
> > > > accordingly within bpf_xdp_adjust_tail().
> > >
> > > [ ... ]
> > >
> > > > +static inline struct xdp_buff *xsk_buff_get_tail(struct xdp_buff *first)
> > > > +{
> > > > + struct xdp_buff_xsk *xskb = container_of(first, struct xdp_buff_xsk, xdp);
> > > > + struct xdp_buff_xsk *frag;
> > > > +
> > > > + frag = list_last_entry(&xskb->pool->xskb_list, struct xdp_buff_xsk,
> > > > + xskb_list_node);
> > > > + return &frag->xdp;
> > > > +}
> > > > +
> > >
> > > [ ... ]
> > >
> > > > +static void __shrink_data(struct xdp_buff *xdp, struct xdp_mem_info *mem_info,
> > > > + skb_frag_t *frag, int shrink)
> > > > +{
> > > > + if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
> > > > + struct xdp_buff *tail = xsk_buff_get_tail(xdp);
> > > > +
> > > > + if (tail)
> > > > + tail->data_end -= shrink;
> > > > + }
> > > > + skb_frag_size_sub(frag, shrink);
> > > > +}
> > > > +
> > > > +static bool shrink_data(struct xdp_buff *xdp, skb_frag_t *frag, int shrink)
> > > > +{
> > > > + struct xdp_mem_info *mem_info = &xdp->rxq->mem;
> > > > +
> > > > + if (skb_frag_size(frag) == shrink) {
> > > > + struct page *page = skb_frag_page(frag);
> > > > + struct xdp_buff *zc_frag = NULL;
> > > > +
> > > > + if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
> > > > + zc_frag = xsk_buff_get_tail(xdp);
> > > > +
> > > > + if (zc_frag) {
> > >
> > > Based on the xsk_buff_get_tail(), would zc_frag ever be NULL?
> >
> > Hey Martin thanks for taking a look, I had to do this in order to satisfy
> > !CONFIG_XDP_SOCKETS builds :/
>
> There is compilation/checker warning if it does not check for NULL?
>
> hmm... but it still should not reach here in the runtime and call
> xsk_buff_get_tail() in the !CONFIG_XDP_SOCKETS build. Can the NULL test on
> the get_tail() return value be removed? The above "mem_info->type ==
> MEM_TYPE_XSK_BUFF_POOL" should have avoided the get_tail() call for the
> !CONFIG_XDP_SOCKETS build. Otherwise, it could be passing NULL to the
> __xdp_return() and hit the same bug again. The NULL check here is pretty
> hard to reason logically.
Thanks for bringing this up, you are of course right. I'll address that.
>
> >
> > >
> > > > + xdp_buff_clear_frags_flag(zc_frag);
> > > > + xsk_buff_del_tail(zc_frag);
> > > > + }
> > > > + }
> > > > +
> > > > + __xdp_return(page_address(page), mem_info, false, zc_frag);
> > >
> > > and iiuc, this patch is fixing a bug when zc_frag is NULL and
> > > MEM_TYPE_XSK_BUFF_POOL.
> >
> > Generally I don't see the need for xdp_return_buff() (which calls in the
> > end __xdp_return() being discussed) to handle MEM_TYPE_XSK_BUFF_POOL, this
> > could be refactored later and then probably this fix would look different,
> > but this is out of the scope now.
> >
> > >
> > > > + return true;
> > > > + }
> > > > + __shrink_data(xdp, mem_info, frag, shrink);
> > > > + return false;
> > > > +}
> > > > +
> > >
> > >
> >
>
>
