Re: [PATCH] Check fops_get() return value

[Takashi Iwai <tiwai@suse.de>]

At Thu, 16 Oct 2008 15:02:40 +0200,
Laurent Pinchart wrote:
> 
> Several subsystem open handlers dereference the fops_get() return value
> without checking it for nullness. This opens a race condition between the
> open handler and module unloading.
> 
> A module can be marked as being unloaded (MODULE_STATE_GOING) before its exit
> function is called and gets the chance to unregister the driver. During that
> window open handlers can still be called, and fops_get() will fail in
> try_module_get() and return a NULL pointer.
> 
> This change checks the fops_get() return value and returns -ENODEV if NULL.
> 
> Reported-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@skynet.be>
> ---
>  drivers/gpu/drm/drm_fops.c          |    4 ++++
>  drivers/media/dvb/dvb-core/dvbdev.c |    5 +++++
>  sound/core/sound.c                  |    4 ++++
>  3 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 851a53f..ef5c295 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -186,6 +186,10 @@ int drm_stub_open(struct inode *inode, struct file *filp)
>  
>  	old_fops = filp->f_op;
>  	filp->f_op = fops_get(&dev->driver->fops);
> +	if (filp->f_op == NULL) {

I think the more standard style is

 if (!file->fop) {

Except for that minor issue, it looks good to me.
In any case,

Acked-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> +		filp->f_op = old_fops;
> +		goto out;
> +	}
>  	if (filp->f_op->open && (err = filp->f_op->open(inode, filp))) {
>  		fops_put(filp->f_op);
>  		filp->f_op = fops_get(old_fops);
> diff --git a/drivers/media/dvb/dvb-core/dvbdev.c b/drivers/media/dvb/dvb-core/dvbdev.c
> index e713277..fb4e241 100644
> --- a/drivers/media/dvb/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb/dvb-core/dvbdev.c
> @@ -85,6 +85,10 @@ static int dvb_device_open(struct inode *inode, struct file *file)
>  		file->private_data = dvbdev;
>  		old_fops = file->f_op;
>  		file->f_op = fops_get(dvbdev->fops);
> +		if (file->f_op == NULL) {
> +			file->f_op = old_fops;
> +			goto fail;
> +		}
>  		if(file->f_op->open)
>  			err = file->f_op->open(inode,file);
>  		if (err) {
> @@ -95,6 +99,7 @@ static int dvb_device_open(struct inode *inode, struct file *file)
>  		unlock_kernel();
>  		return err;
>  	}
> +fail:
>  	unlock_kernel();
>  	return -ENODEV;
>  }
> diff --git a/sound/core/sound.c b/sound/core/sound.c
> index c0685e2..8659b84 100644
> --- a/sound/core/sound.c
> +++ b/sound/core/sound.c
> @@ -152,6 +152,10 @@ static int __snd_open(struct inode *inode, struct file *file)
>  	}
>  	old_fops = file->f_op;
>  	file->f_op = fops_get(mptr->f_ops);
> +	if (file->f_op == NULL) {
> +		file->f_op = old_fops;
> +		return -ENODEV;
> +	}
>  	if (file->f_op->open)
>  		err = file->f_op->open(inode, file);
>  	if (err) {
> -- 
> 1.5.0
>