Re: [PATCH 17/17] mtd: rawnand: nandsim: Reorganize ns_cleanup_module()

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote on Mon, 25 May 2020
09:23:15 +0200:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Mon, 25 May
> 2020 08:47:35 +0200:
> 
> > On Mon, 25 May 2020 08:46:37 +0200
> > Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> >   
> > > On Mon, 25 May 2020 00:13:28 +0200
> > > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> > >     
> > > > Hi Richard,
> > > > 
> > > > Richard Weinberger <richard.weinberger@xxxxxxxxx> wrote on Sun, 24 May
> > > > 2020 23:37:13 +0200:
> > > >       
> > > > > On Sat, May 9, 2020 at 9:19 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:        
> > > > > >         ns_debugfs_remove(ns);
> > > > > > -       ns_free(ns);    /* Free nandsim private resources */
> > > > > > -       nand_release(chip); /* Unregister driver */
> > > > > > -       kfree(ns);        /* Free other structures */
> > > > > > -       ns_free_lists();
> > > > > > +       WARN_ON(mtd_device_unregister(nsmtd));
> > > > > > +       ns_free(ns);
> > > > > > +       kfree(erase_block_wear);
> > > > > > +       nand_cleanup(chip);
> > > > > > +       list_for_each_safe(pos, n, &grave_pages) {
> > > > > > +               kfree(list_entry(pos, struct grave_page, list));
> > > > > > +               list_del(pos);          
> > > > > 
> > > > > Are you sure you can use pos after freeing the entry?
> > > > > Smells like use after free.
> > > > >         
> > > > 
> > > > Mmmmh, I should probably invert those two lines, first call list_del()
> > > > and then call kfree() on list_entry().      
> > > 
> > > You can also use  list_for_each_entry_safe():  
> 
> I usually use this helper, but I guess I copy/pasted the below lines
> from somewhere else in this file... I'll use list_for_each_entry_safe().

Actually, grave_pages, weak_pages and weak_blocks are three structures
of different types, that's why they called kfree() directly on
list_entry() -> to avoid having to declare 6 different pointers. I'll
stick to the same presentation than ns_free_lists then.

> 
> > > 
> > > 	struct grave_page *pos, *n;
> > > 
> > > 	...
> > > 
> > > 	list_for_each_safe(pos, n, &grave_pages, list) {    
> > 
> > 	list_for_each_entry_safe(pos, n, &grave_pages, list) {
> >   
> > > 		list_del(&pos->list);
> > > 		kfree(pos);
> > > 	}
> > >     
> > > > 
> > > > Thanks for noticing!
> > > > Miquèl      
> > >     
> >   
> 
> Thanks,
> Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/