[CRIU] [PATCH] mount: mnt_entry_alloc -- Don't forget to init @mnt_slave

[Andrew Vagin]

On Tue, Sep 01, 2015 at 01:12:07PM +0300, Cyrill Gorcunov wrote:
> On Tue, Sep 01, 2015 at 12:32:31PM +0300, Pavel Emelyanov wrote:
> > > 
> > > Because it's a separate routine called "mnt_entry_alloc",
> > > it has no clue how would we use any of this list entries.
> > > So I think it's better to init this entry, frankly its
> > > just a few cpu ops.
> > 
> > Kernel doesn't initialize list-entries without need either :) Only
> > heads. So I'd just add a comment somewhere saying this.
> 
> static struct mount *alloc_vfsmnt(const char *name)
> {
> 	struct mount *mnt = kmem_cache_zalloc(mnt_cache, GFP_KERNEL);
> 	...
> 		INIT_LIST_HEAD(&mnt->mnt_share);
> 		INIT_LIST_HEAD(&mnt->mnt_slave_list);
> 		INIT_LIST_HEAD(&mnt->mnt_slave);
> 	...
> }
> 
> Really, such kind of things are simply waiting for trigger the bugs.
> Instead of adding comment do a right thing and simply init the entry.

"Null pointer dereference" is better than wrong data in images.

> Yes, we have a couple of places in criu where we don't init list
> entries but the difference is that we allocate and add them immediately,
> not somewhere "later" in code. For example
> 
> open_remap_ghost
> 	gf = shmalloc(sizeof(*gf));
> 	...
> 	list_add_tail(&gf->list, &ghost_files);
> 
> that's fine, because we allocate and collect in one place.
> 
> Anyway, this aspect doesn't worth to waste time on it so if
> you both don't like it I can live with that, drop it.
> 
> 	Cyrill