Re: [PATCH] sdio: pass unknown cis tuples to sdio drivers

[Albert Herranz <albert_herranz-mRCrAkd8dF0@public.gmane.org>]

--- El vie, 11/9/09, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> escribió:
> > Some manufacturers provide vendor information in
> non-vendor specific CIS
> > tuples. For example, Broadcom uses an Extended
> Function tuple to provide
> > the MAC address on some of their network cards, as in
> the case of the
> > Nintendo Wii WLAN daughter card.
> > 
> > This patch allows passing correct tuples unknown to
> the SDIO core to
> > a matching SDIO driver instead of rejecting them and
> failing.
> > 
> 
> This looks leaky to me.
> 

Hi Andrew, thanks for the review.
I hope I can clarify a bit what the patch does in the next comments.

> 
> :         if (i < ARRAY_SIZE(cis_tpl_list)) {
> :             const struct cis_tpl *tpl = cis_tpl_list + i;
> :             if (tpl_link < tpl->min_size) {
> :                 printk(KERN_ERR
> :                        "%s: bad CIS tuple 0x%02x"
> :                        " (length = %u, expected >= %u)\n",
> :                        mmc_hostname(card->host),
> :                        tpl_code, tpl_link, tpl->min_size);
> :                 ret = -EINVAL;
> 
> ret == -EINVAL
> 

At this point ret is not -EINVAL. If it was -EINVAL the code would have had exit at this snipped before:

                if (ret) {
                        kfree(this);
                        break;
                }


> :             } else if (tpl->parse) {
> :                 ret = tpl->parse(card, func,
> :                                  this->data, tpl_link);
> :             }
> :             /* already successfully parsed, not needed anymore */
> :             if (!ret)
> :                 kfree(this);
> 
> `this' doesn't get freed
> 

Yes, that's the whole point of the patch. It must be freed _only_ if the SDIO core parsed it. If the SDIO core cannot parse it then it gets passed to the SDIO driver via the SDIO func struct tuple list (later).

> :         } else {
> :            /* unknown tuple */
> :            ret = -EILSEQ;
> :         }
> :
> :         if (ret == -EILSEQ) {
> 
> `this' doesn't get remembered.
> 

When ret is -EILSEQ `this' is linked to the SDIO func tuple list (later).

> :            /* this tuple is unknown to the core */
> :            this->next = NULL;
> :            this->code = tpl_code;
> :            this->size = tpl_link;
> :            *prev = this;
> :            prev = &this->next;
> :            pr_debug("%s: queuing CIS tuple 0x%02x length %u\n",
> :                     mmc_hostname(card->host), tpl_code, tpl_link);
> :            /* keep on analyzing tuples */
> :            ret = 0;
> :         }
> : 
> :         ptr += tpl_link;
> 
> `this' leaks.
> 

`this' doesn't leak. `this' has been linked to the SDIO func tuple list in:

*prev = this;


> :     } while (!ret);
> 
> Please check?
> 

Thanks a lot for you comments,
Albert



      
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html