On 24.07.2015 21:12, Kalle Valo wrote: > Mike Looijmans <mike.looijmans@xxxxxxxx> writes: > >> On 24-07-15 13:35, Alexey Khoroshilov wrote: >>> On 24.07.2015 18:02, Mike Looijmans wrote: >>>> On 24-07-15 10:39, Alexey Khoroshilov wrote: >>>>> Dear Mike, >>>>> >>>>> On 24.07.2015 14:01, Mike Looijmans wrote: >>>>>> Regarding this commit: >>>>>> >>>>>> https://lkml.org/lkml/2014/12/12/709 >>>>>> >>>>>> rsi: fix memory leak in rsi_load_ta_instructions() >>>>>> >>>>>> Memory allocated by kmemdup() in rsi_load_ta_instructions() is >>>>>> leaked. >>>>>> But duplication of firmware data here is useless, >>>>>> so the patch removes kmemdup() at all. >>>>>> >>>>>> Found by Linux Driver Verification project (linuxtesting.org). >>>>>> >>>>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx> >>>>>> Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxx> >>>>>> >>>>>> We use this driver for the Redpine Wifi chip on our "florida" board, and >>>>>> after this commit it stopped working. Symptom was that the "wlan0" >>>>>> device was not created at all. Reverting the commit makes it work again. >>>>>> >>>>>> Apparently, the kmemdup action is needed for something. I suspect the >>>>>> DMA controller is still copying the firmware data before the method >>>>>> returned. >>>>> >>>>> To test your hypothesis, could you please check if it is still broken >>>>> with kfree(fw); added just after release_firmware(fw_entry); in >>>>> rsi_load_ta_instructions(). >>>> >>>> Tried, and appears to work if i just kfree() the firmware copy. It does >>>> leave a bad taste though. I'd expect fw_entry->data to point to a >>>> kmalloc'd area as well. So it might work now just because it happens to >>>> be that the memory if "far enough away" and isn't being touched by >>>> anything else until the transfer is done. And on some other setup, it >>>> may suddenly fail unexpectedly. >>>> >>>> I thought to move the kfree to a point where the driver unregisters, but >>>> apparently it doesn't have any internal hook for that (sdio_done or so). >>>> >>>> I'd really like to see some comment from the Redpine folks on this, but >>>> since there hasn't been any event in the past year or so, I don't expect >>>> much. >>> >>> May be if firmware comes from userspace it is mapped to both kernel and >>> userspace and by some reason it is not good for DMA. >>> Another idea is fw_entry->data appears to be misaligned somehow. >> >> Just read some documentation: >> https://kernel.org/doc/Documentation/firmware_class/README >> It states "kernel: grows a buffer in PAGE_SIZE increments to hold the >> image as it comes in". Probably the firmware buffer is fragmented in >> memory as a result, and that wouldn't be very DMA friendly indeed. The >> copy would be contiguous again. Interesting idea. Even fw_get_filesystem_firmware() uses vmalloc(), so fw_entry->data may be physically noncontiguous in case of reading firmware from file system as well. The only my doubt is whether contiguous memory is required here. As far as I can see, rsi_copy_to_card() writes data by blocks of size dev->tx_blk_size that is 256 for rsi_91x_sdio. So, it is not clear where problems can appear. >> I noticed that the same kfree is missing in the usb glue part of the >> RSI driver. I can't test that though, we only have the SDIO connected. >> >> I can submit a patch to add the "kfree()" call. Don't know about the >> revert though, can I just submit the revert as a patch and then the >> "kfree" as a second patch in the same set? > > Better to do the revert and kfree() addition in one patch, they are > simple enough. Just document in the commit log that it reverts the > broken commit. > > And remember to add CC stable to the commit log so that the fix goes to > stable releases. Agree with Kalle with a couple of notes. To document a fix of a broken commit the preferable way is to use Fixes: tag. https://kernel.org/doc/Documentation/SubmittingPatches Also it would be useful to add a comment before kmemdup() with information why it is needed there. -- Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
