On Mon, Mar 10, 2025 at 10:04:22PM -0700, Matthew Brost wrote:
> 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>
>
Actually, we have another problem too. The 'free_bind_ops' label in
vm_bind_ioctl_check_args() either isn't needed or it should *bind_ops to
NULL after kvfree to avoid a double free in put_vm label in
xe_vm_bind_ioctl().
This patch is still valid though.
Matt
> > regards,
> > dan carpenter
> >
