Hi,
On Mon, Aug 29, 2022 at 3:41 AM Dan Carpenter <dan.carpen...@oracle.com> wrote:
>
> Hello Alexander Aring,
>
> The patch 7a3de7324c2b: "fs: dlm: trace user space callbacks" from
> Aug 15, 2022, leads to the following Smatch static checker warning:
>
> fs/dlm/lock.c:5900 dlm_user_request()
> warn: 'lkb' was already freed.
>
> fs/dlm/lock.c
> 5832 int dlm_user_request(struct dlm_ls *ls, struct dlm_user_args *ua,
> 5833 int mode, uint32_t flags, void *name, unsigned
> int namelen)
> 5834 #endif
> 5835 {
> 5836 struct dlm_lkb *lkb;
> 5837 struct dlm_args args;
> 5838 int error;
> 5839
> 5840 dlm_lock_recovery(ls);
> 5841
> 5842 error = create_lkb(ls, &lkb);
> 5843 if (error) {
> 5844 kfree(ua);
> 5845 goto out;
> 5846 }
> 5847
> 5848 trace_dlm_lock_start(ls, lkb, name, namelen, mode, flags);
> 5849
> 5850 if (flags & DLM_LKF_VALBLK) {
> 5851 ua->lksb.sb_lvbptr = kzalloc(DLM_USER_LVB_LEN,
> GFP_NOFS);
> 5852 if (!ua->lksb.sb_lvbptr) {
> 5853 kfree(ua);
> 5854 __put_lkb(ls, lkb);
> 5855 error = -ENOMEM;
> 5856 goto out_trace_end;
> 5857 }
> 5858 }
> 5859 #ifdef CONFIG_DLM_DEPRECATED_API
> 5860 error = set_lock_args(mode, &ua->lksb, flags, namelen,
> timeout_cs,
> 5861 fake_astfn, ua, fake_bastfn, &args);
> 5862 #else
> 5863 error = set_lock_args(mode, &ua->lksb, flags, namelen,
> fake_astfn, ua,
> 5864 fake_bastfn, &args);
> 5865 #endif
> 5866 if (error) {
> 5867 kfree(ua->lksb.sb_lvbptr);
> 5868 ua->lksb.sb_lvbptr = NULL;
> 5869 kfree(ua);
> 5870 __put_lkb(ls, lkb);
> 5871 goto out_trace_end;
> 5872 }
> 5873
> 5874 /* After ua is attached to lkb it will be freed by
> dlm_free_lkb().
> 5875 When DLM_IFL_USER is set, the dlm knows that this is a
> userspace
> 5876 lock and that lkb_astparam is the dlm_user_args
> structure. */
> 5877 lkb->lkb_flags |= DLM_IFL_USER;
> 5878 error = request_lock(ls, lkb, name, namelen, &args);
> 5879
> 5880 switch (error) {
> 5881 case 0:
> 5882 break;
> 5883 case -EINPROGRESS:
> 5884 error = 0;
> 5885 break;
> 5886 case -EAGAIN:
> 5887 error = 0;
> 5888 fallthrough;
> 5889 default:
> 5890 __put_lkb(ls, lkb);
> 5891 goto out_trace_end;
> 5892 }
> 5893
> 5894 /* add this new lkb to the per-process list of locks */
> 5895 spin_lock(&ua->proc->locks_spin);
> 5896 hold_lkb(lkb);
> 5897 list_add_tail(&lkb->lkb_ownqueue, &ua->proc->locks);
> 5898 spin_unlock(&ua->proc->locks_spin);
> 5899 out_trace_end:
> --> 5900 trace_dlm_lock_end(ls, lkb, name, namelen, mode, flags,
> error, false);
> ^^^
> This is freed, but probably the trace code doesn't care? I'm not sure.
>
Thanks. It cares, there is currently an issue. We need to rearrange
the code there so we don't have any use after free here.
However the user has to opt-in.
- Alex
