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?
>
> 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.
regards,
dan carpenter
