Re: [PATCH v6 27/37] sg: add sg v4 interface support

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-01-13 9:28 a.m., kbuild test robot wrote:
> Hi Douglas,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on scsi/for-next]
> [also build test WARNING on linus/master v5.5-rc6]
> [cannot apply to mkp-scsi/for-next next-20200110]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/Douglas-Gilbert/sg-add-v4-interface/20200113-080059
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> New smatch warnings:
> drivers/scsi/sg.c:504 sg_open() error: potential null dereference 'sfp'.  (sg_add_sfp returns null)
> 
> Old smatch warnings:
> drivers/scsi/sg.c:3238 sg_find_srp_by_id() warn: statement has no effect 3

__attribute__((nonnull(<n>))) only applies to function arguments, not its
return value. So there seems no way to tell the compiler: this function,
that returns a pointer, should/will never return the NULL pointer; feel
free to check that.

sg_add_sfp() is such a function, if it fails its retval is either a valid
pointer _or_ IS_ERR(retval) is true. It follows that sfp->tid on line 504
can never be a null dereference.

Doug Gilbert

> 
> vim +/sfp +504 drivers/scsi/sg.c
> 
>     433	
>     434	/*
>     435	 * Corresponds to the open() system call on sg devices. Implements O_EXCL on
>     436	 * a per device basis using 'open_cnt'. If O_EXCL and O_NONBLOCK and there is
>     437	 * already a sg handle open on this device then it fails with an errno of
>     438	 * EBUSY. Without the O_NONBLOCK flag then this thread enters an interruptible
>     439	 * wait until the other handle(s) are closed.
>     440	 */
>     441	static int
>     442	sg_open(struct inode *inode, struct file *filp)
>     443	{
>     444		bool o_excl, non_block;
>     445		int min_dev = iminor(inode);
>     446		int op_flags = filp->f_flags;
>     447		int res;
>     448		__maybe_unused int o_count;
>     449		struct sg_device *sdp;
>     450		struct sg_fd *sfp;
>     451	
>     452		nonseekable_open(inode, filp);
>     453		o_excl = !!(op_flags & O_EXCL);
>     454		non_block = !!(op_flags & O_NONBLOCK);
>     455		if (o_excl && ((op_flags & O_ACCMODE) == O_RDONLY))
>     456			return -EPERM;/* not permitted, need write access for O_EXCL */
>     457		sdp = sg_get_dev(min_dev);	/* increments sdp->d_ref */
>     458		if (IS_ERR(sdp))
>     459			return PTR_ERR(sdp);
>     460	
>     461		/* Prevent the device driver from vanishing while we sleep */
>     462		res = scsi_device_get(sdp->device);
>     463		if (res)
>     464			goto sg_put;
>     465		res = scsi_autopm_get_device(sdp->device);
>     466		if (res)
>     467			goto sdp_put;
>     468		res = sg_allow_if_err_recovery(sdp, non_block);
>     469		if (res)
>     470			goto error_out;
>     471	
>     472		mutex_lock(&sdp->open_rel_lock);
>     473		if (op_flags & O_NONBLOCK) {
>     474			if (o_excl) {
>     475				if (atomic_read(&sdp->open_cnt) > 0) {
>     476					res = -EBUSY;
>     477					goto error_mutex_locked;
>     478				}
>     479			} else {
>     480				if (SG_HAVE_EXCLUDE(sdp)) {
>     481					res = -EBUSY;
>     482					goto error_mutex_locked;
>     483				}
>     484			}
>     485		} else {
>     486			res = sg_wait_open_event(sdp, o_excl);
>     487			if (res) /* -ERESTARTSYS or -ENODEV */
>     488				goto error_mutex_locked;
>     489		}
>     490	
>     491		/* N.B. at this point we are holding the open_rel_lock */
>     492		if (o_excl)
>     493			set_bit(SG_FDEV_EXCLUDE, sdp->fdev_bm);
>     494	
>     495		o_count = atomic_inc_return(&sdp->open_cnt);
>     496		sfp = sg_add_sfp(sdp);		/* increments sdp->d_ref */
>     497		if (IS_ERR(sfp)) {
>     498			atomic_dec(&sdp->open_cnt);
>     499			res = PTR_ERR(sfp);
>     500			goto out_undo;
>     501		}
>     502	
>     503		filp->private_data = sfp;
>   > 504		sfp->tid = (current ? current->pid : -1);
>     505		mutex_unlock(&sdp->open_rel_lock);
>     506		SG_LOG(3, sfp, "%s: minor=%d, op_flags=0x%x; %s count after=%d%s\n",
>     507		       __func__, min_dev, op_flags, "device open", o_count,
>     508		       ((op_flags & O_NONBLOCK) ? " O_NONBLOCK" : ""));
>     509	
>     510		res = 0;
>     511	sg_put:
>     512		kref_put(&sdp->d_ref, sg_device_destroy);
>     513		/* if success, sdp->d_ref is incremented twice, decremented once */
>     514		return res;
>     515	
>     516	out_undo:
>     517		if (o_excl) {		/* undo if error */
>     518			clear_bit(SG_FDEV_EXCLUDE, sdp->fdev_bm);
>     519			wake_up_interruptible(&sdp->open_wait);
>     520		}
>     521	error_mutex_locked:
>     522		mutex_unlock(&sdp->open_rel_lock);
>     523	error_out:
>     524		scsi_autopm_put_device(sdp->device);
>     525	sdp_put:
>     526		scsi_device_put(sdp->device);
>     527		goto sg_put;
>     528	}
>     529	
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
>