On Mon, Mar 10, 2025 at 09:22:50PM +0300, Dan Carpenter wrote:
> On Mon, Mar 10, 2025 at 12:56:46PM -0400, Rodrigo Vivi wrote:
> > On Mon, Mar 10, 2025 at 01:48:00PM +0300, Dan Carpenter wrote:
> > > The error handling assumes that vm_bind_ioctl_check_args() will
> > > initialize "bind_ops" but there are a couple early returns where that's
> > > not true. Initialize "bind_ops" to NULL from the start.
> >
> > It is not a couple, but only the one goto put_vm where this bind_ops
> > gets actually initialized, or not...
> >
>
> I'm on linux-next. I'm not seeing the goto put_vm... I think we're
> looking at different code.
>
> 3063 static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
> 3064 struct drm_xe_vm_bind *args,
> 3065 struct drm_xe_vm_bind_op **bind_ops)
> 3066 {
> 3067 int err;
> 3068 int i;
> 3069
> 3070 if (XE_IOCTL_DBG(xe, args->pad || args->pad2) ||
> 3071 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> 3072 return -EINVAL;
>
> One.
>
> 3073
> 3074 if (XE_IOCTL_DBG(xe, args->extensions))
> 3075 return -EINVAL;
>
> Two.
>
> 3076
> 3077 if (args->num_binds > 1) {
> 3078 u64 __user *bind_user =
> 3079 u64_to_user_ptr(args->vector_of_binds);
> 3080
> 3081 *bind_ops = kvmalloc_array(args->num_binds,
>
> Initialized.
>
> 3082 sizeof(struct drm_xe_vm_bind_op),
> 3083 GFP_KERNEL | __GFP_ACCOUNT |
> 3084 __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> 3085 if (!*bind_ops)
> 3086 return args->num_binds > 1 ? -ENOBUFS : -ENOMEM;
> 3087
> 3088 err = __copy_from_user(*bind_ops, bind_user,
> 3089 sizeof(struct drm_xe_vm_bind_op) *
> 3090 args->num_binds);
> 3091 if (XE_IOCTL_DBG(xe, err)) {
> 3092 err = -EFAULT;
> 3093 goto free_bind_ops;
> 3094 }
> 3095 } else {
> 3096 *bind_ops = &args->bind;
> 3097 }
>
> > but perhaps the order in the exit is wrong and we should move the
> > kvfree(bind_ops) upper to the end of put_exec_queue?
> >
> > Matt, thoughts on the order here?
> >
Rodrigo – I think you are looking in the wrong spot in the code. Dan's
subsequent reply, I believe, explains the issue correctly, so I think
the patch is good.
> > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
>
> I feel like ideally vm_bind_ioctl_check_args() would clean up after
> itself on failure and, yes, it should be in reverse order from how
> it was allocated.
>
This is a bit of goofy error handling/convention—perhaps it could be
cleaned up in a follow-up.
That said, this patch is correct. However, the 'Fixes' tag looks
wrong—it has been broken for a bit longer than the tagged patch. We can
fix it upon merge.
With that:
Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> regards,
> dan carpenter
>
