On Thu, Mar 31, 2022 at 04:21:45PM +0800, Xiaoke Wang wrote:
> On Thu 31 Mar 2022 15:36:21 +0800, dan.carpenter@xxxxxxxxxx wrote:
> >> @@ -134,7 +134,12 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct adapter *padapter)
> >> msleep(10);
> >> res = rtw_os_xmit_resource_alloc(padapter, pxmitbuf, (MAX_XMITBUF_SZ + XMITBUF_ALIGN_SZ));
> >> if (res == _FAIL) {
> >> - goto exit;
> >> + pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> >> + for (; i >= 0; i--) {
> >
> > This frees one more element than you intended. It should be:
> >
> > while (--i >= 0) {
> >
>
> In fact, this is considering that we do not know where is the failure
> from. In rtw_os_xmit_resource_alloc(), the failure can from
>
> > pxmitbuf->pallocated_buf = kzalloc(alloc_sz, GFP_KERNEL);
>
> , but also can from
>
> > pxmitbuf->pxmit_urb[i] = usb_alloc_urb(0, GFP_KERNEL);
>
> So if we do not handle the current failed item and just skip it, then some
> memory may be ignored.
The rtw_os_xmit_resource_alloc() function should clean up after itself
and not leave things partially allocated.
First of all MAX_XMITBUF_SZ is 20480 bytes. It's giant. We need to
figure out what's up with that. But then if "pxmitbuf->pxmit_urb[i] =
usb_alloc_urb(0, GFP_KERNEL);" fails, we allocate directly over the
pxmitbuf->pallocated_buf on the second attempt. So it leaks memory.
Every function should clean up after itself. No partial allocations.
That always leads to bugs.
regards,
dan carpenter
