[Ocfs2-devel] [BUG] ocfs2/dlm: possible data races in dlm_drop_lockres_ref_done() and dlm_get_lock_resource()

[Tuo Li]

Hello,

Thanks for your reply! It is very helpful!

Do you mean in dlm_wait_for_lock_mastery()?
>

Yes, it is dlm_wait_for_lock_mastery(). I am sorry to confuse you.

Even if owner changes suddenly, it will recheck, so I think it is also
> fine.


Does 'recheck' here mean if owner changes, it will go to the label
'recheck' at Line 1011.
If so, when rechecking, the race can occur again at Line 1023. And thus can
cause infinite rechecking in extreme cases.

Thanks,
Tuo Li

On Fri, Jun 16, 2023 at 10:11 AM Joseph Qi <joseph.qi at linux.alibaba.com>
wrote:

> Hi,
>
> On 6/13/23 4:23 PM, Tuo Li wrote:
> > Hello,
> >
> > Our static analysis tool finds some possible data races in the OCFS2 file
> > system in Linux 6.4.0-rc6.
> >
> > In most calling contexts, the variables  such as res->lockname.name and
> > res->owner are accessed with holding the lock res->spinlock. Here is an
> > example:
> >
> >   lockres_seq_start() --> Line 539 in dlmdebug.c
> >     spin_lock(&res->spinlock); --> Line 574 in dlmdebug.c (Lock
> > res->spinlock)
> >     dump_lockres(res, ...); --> Line 575 in fs/ocfs2/dlm/dlmdebug.c
> >       stringify_lockname(res->lockname.name, ...);  --> Line 493 in
> > dlmdebug.c (Access res->lockname.name)
> >       scnprintf(..., res->owner, ...);  -->Line 498 in dlmdebug.c (Access
> > res->owner)
> >
> > However, in the following calling contexts:
> >
> >   dlm_deref_lockres_worker() --> Line 2439 in dlmmaster.c
> >     dlm_drop_lockres_ref_done() --> Line 2459 in dlmmaster.c
> >       lockname = res->lockname.name; --> Line 2416 in dlmmaster.c
> (Access
> > res->lockname.name)
>
> lockname won't changed during the lockres lifecycle.
> So this won't cause any real problem since now it holds a reference.
>
> >
> >   dlm_get_lock_resource() --> Line 701 in dlmmaster.c
> >     if (res->owner != dlm->node_num) --> Line 1023 in dlmmaster.c (Access
> > res->owner)
>
> Do you mean in dlm_wait_for_lock_mastery()?
> Even if owner changes suddenly, it will recheck, so I think it is also
> fine.
>
> Thanks,
> Joseph
>
> >
> > The variables res->lockname.name and res->owner are accessed
> respectively
> > without holding the lock res->spinlock, and thus data races can occur.
> >
> > I am not quite sure whether these possible data races are real and how to
> > fix
> > them if they are real.
> >
> > Any feedback would be appreciated, thanks!
> >
> > Reported-by: BassCheck <bass at buaa.edu.cn> <bass at buaa.edu.cn>
> >
> > Best wishes,
> > Tuo Li
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20230616/91c5a76b/attachment.html>