Re: [PATCH v4 11/14] drm/panfrost: Add a PANFROST_SYNC_BO ioctl

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 16 Oct 2025 12:02:10 +0200
Marcin Ślusarz <marcin.slusarz@arm.com> wrote:

> On Thu, Oct 16, 2025 at 11:52:24AM +0200, Boris Brezillon wrote:
> > On Thu, 16 Oct 2025 10:42:21 +0200
> > Marcin Ślusarz <marcin.slusarz@arm.com> wrote:
> >   
> > > On Wed, Oct 15, 2025 at 06:03:23PM +0200, Boris Brezillon wrote:  
> > > > +static int panfrost_ioctl_sync_bo(struct drm_device *ddev, void *data,
> > > > +				  struct drm_file *file)
> > > > +{
> > > > +	struct drm_panfrost_sync_bo *args = data;
> > > > +	struct drm_panfrost_bo_sync_op *ops;
> > > > +	struct drm_gem_object *obj;
> > > > +	int ret;
> > > > +	u32 i;
> > > > +
> > > > +	if (args->pad)
> > > > +		return -EINVAL;
> > > > +
> > > > +	ops = kvmalloc_array(args->op_count, sizeof(*ops), GFP_KERNEL);
> > > > +	if (!ops) {
> > > > +		DRM_DEBUG("Failed to allocate incoming BO sync ops array\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	if (copy_from_user(ops, (void __user *)(uintptr_t)args->ops,
> > > > +			   args->op_count * sizeof(*ops))) {
> > > > +		DRM_DEBUG("Failed to copy in BO sync ops\n");
> > > > +		ret = -EFAULT;
> > > > +		goto err_ops;
> > > > +	}
> > > > +
> > > > +	for (i = 0; i < args->op_count; i++) {    
> > > 
> > > If args->op_count is 0, if I'm not mistaken, kvmalloc_array and
> > > copy_to_user will succeed, but then this function will return
> > > unitialized value. Maybe add an explicit check for op_count == 0
> > > at the beginning and avoid going through all that code?  
> > 
> > If args->op_count=0 the loop would be exited right away, so I'm not too
> > sure where the problem is.  
> 
> Maybe I didn't explain it correctly, so let me clear that up:
> ret is not initialized and not set anywhere if op_count is 0.

Ah, right. I overlooked the fact ret wasn't initialized. Sorry for the
noise.

> 
> > This being said, I agree it's not worth
> > going through kvmalloc_array() and copy_from_user() if we know there's
> > nothing to do. And it's probably a bit fragile to rely on
> > kvmalloc_array() not returning NULL when the size is zero (I actually
> > thought it was), so I agree we'd rather bail out early in that case.
> >   
> > >   
> > > > +		if (ops[i].flags & ~PANFROST_BO_SYNC_OP_FLAGS) {
> > > > +			ret = -EINVAL;
> > > > +			goto err_ops;
> > > > +		}
> > > > +
> > > > +		obj = drm_gem_object_lookup(file, ops[i].handle);
> > > > +		if (!obj) {
> > > > +			ret = -ENOENT;
> > > > +			goto err_ops;
> > > > +		}
> > > > +
> > > > +		ret = panfrost_gem_sync(obj, ops[i].flags,
> > > > +					ops[i].offset, ops[i].size);
> > > > +
> > > > +		drm_gem_object_put(obj);
> > > > +
> > > > +		if (ret)
> > > > +			goto err_ops;
> > > > +	}
> > > > +
> > > > +err_ops:
> > > > +	kvfree(ops);
> > > > +
> > > > +	return ret;
> > > > +}    
> >