| Message ID | 87y3072iib.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: cleanup patches for soc-core | expand |
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 40bac40..00887f7 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -171,7 +171,10 @@ static void soc_init_component_debugfs(struct snd_soc_component *component) > > static void soc_cleanup_component_debugfs(struct snd_soc_component *component) > { > + if (!component->debugfs_root) > + return; that test is redundant, it's safe to call debugfs_remove_recursive() without checking the argument (done internally). > debugfs_remove_recursive(component->debugfs_root); > + component->debugfs_root = NULL; > }
On Tue, Aug 06, 2019 at 09:49:20AM -0500, Pierre-Louis Bossart wrote: > > { > > + if (!component->debugfs_root) > > + return; > that test is redundant, it's safe to call debugfs_remove_recursive() without > checking the argument (done internally). It's not completely redundant... > > debugfs_remove_recursive(component->debugfs_root); > > + component->debugfs_root = NULL; > > } ...with this, the two in combination add protection against a double free. Not 100% sure it's worth it but I queued the patch since I couldn't strongly convince myself it's a bad idea.
On 8/6/19 11:22 AM, Mark Brown wrote: > On Tue, Aug 06, 2019 at 09:49:20AM -0500, Pierre-Louis Bossart wrote: >>> { >>> + if (!component->debugfs_root) >>> + return; > >> that test is redundant, it's safe to call debugfs_remove_recursive() without >> checking the argument (done internally). > > It's not completely redundant... > >>> debugfs_remove_recursive(component->debugfs_root); >>> + component->debugfs_root = NULL; >>> } > > ...with this, the two in combination add protection against a double > free. Not 100% sure it's worth it but I queued the patch since I > couldn't strongly convince myself it's a bad idea. I was only referring to the first test, which will be duplicated. see below. The second part is just fine. /** * debugfs_remove_recursive - recursively removes a directory * @dentry: a pointer to a the dentry of the directory to be removed. If this * parameter is NULL or an error value, nothing will be done. * * This function recursively removes a directory tree in debugfs that * was previously created with a call to another debugfs function * (like debugfs_create_file() or variants thereof.) * * This function is required to be called in order for the file to be * removed, no automatic cleanup of files will happen when a module is * removed, you are responsible here. */ void debugfs_remove_recursive(struct dentry *dentry) { struct dentry *child, *parent; if (IS_ERR_OR_NULL(dentry)) return;
On Tue, Aug 06, 2019 at 11:46:28AM -0500, Pierre-Louis Bossart wrote: > On 8/6/19 11:22 AM, Mark Brown wrote: > > ...with this, the two in combination add protection against a double > > free. Not 100% sure it's worth it but I queued the patch since I > > couldn't strongly convince myself it's a bad idea. > I was only referring to the first test, which will be duplicated. see below. > The second part is just fine. > { > struct dentry *child, *parent; > > if (IS_ERR_OR_NULL(dentry)) > return; Oh, I see it's got a NULL test in it. TBH I don't think it hurts to check in the caller, it avoids someone having to check to make sure that the NULL check is in the function.
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 40bac40..00887f7 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -171,7 +171,10 @@ static void soc_init_component_debugfs(struct snd_soc_component *component) static void soc_cleanup_component_debugfs(struct snd_soc_component *component) { + if (!component->debugfs_root) + return; debugfs_remove_recursive(component->debugfs_root); + component->debugfs_root = NULL; } static int dai_list_show(struct seq_file *m, void *v)