linux-kernel - Re: 回复: [PATCH] workqueue: Make a warning when a pending delay work being re-init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dc50c32a-9137-47a6-8715-486a3e5c43a1@t-8ch.de>
Date:   Thu, 16 Nov 2023 05:13:32 +0100
From:   Thomas Weißschuh <thomas@...ch.de>
To:     黄再漾(Joyyoung Huang) 
        <huangzaiyang@...o.com>
Cc:     "tj@...nel.org" <tj@...nel.org>,
        "jiangshanlai@...il.com" <jiangshanlai@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        ZaiYang Huang <hzy0719@....com>,
        Mukesh Ojha <quic_mojha@...cinc.com>
Subject: Re: 回复: [PATCH] workqueue:
 Make a warning when a pending delay work being re-init

On 2023-11-16 01:53:30+0000, 黄再漾(Joyyoung Huang) wrote:
> 
> On 2023-11-15 19:34:27+0800, huangzaiyang wrote:
> > There is a timer_list race condition if a delay work is queued twice,
> > this usually won't happen unless someone reinitializes the task before performing the enqueue operation,likeï¼?
> > https://github.com/torvalds/linux/blob/master/drivers/devfreq/devfreq.
> > c#L487 A warning message will help developers identify this irregular
> > usage.
> >
> > Signed-off-by: ZaiYang Huang <hzy0719@....com>
> > ---
> >  include/linux/workqueue.h | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> > index 24b1e5070f4d..54102ed794e5 100644
> > --- a/include/linux/workqueue.h
> > +++ b/include/linux/workqueue.h
> > @@ -266,6 +266,22 @@ static inline void
> > destroy_delayed_work_on_stack(struct delayed_work *work) { }  static
> > inline unsigned int work_static(struct work_struct *work) { return 0;
> > }  #endif
> >
> > +/**
> > + * work_pending - Find out whether a work item is currently pending
> > + * @work: The work item in question
> > + */
> > +#define work_pending(work) \
> > +       test_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))
> > +
> > +/**
> > + * delayed_work_pending - Find out whether a delayable work item is
> > +currently
> > + * pending
> > + * @w: The work item in question
> > + */
> > +#define delayed_work_pending(w) \
> > +       work_pending(&(w)->work)
> > +
> > +
> >  /*
> >   * initialize all of a work item in one go
> >   *
> > @@ -310,6 +326,7 @@ static inline unsigned int work_static(struct
> > work_struct *work) { return 0; }
> >
> >  #define __INIT_DELAYED_WORK(_work, _func, _tflags)                     \
> >         do {                                                            \
> > +               WARN_ON(delayed_work_pending(_work));                   \
> 
> How does this work when the data _work points to is not yet initialized?
> Reading uninitialized data is UB and may spuriously trigger the warning.
> 
> --> The data member in work_struct is a variable of type atomic_long_t, and 0 is the initial value of the atomic variable, right?

Please use proper email quoting style, using "> ".

Normal integers are not initialized by default when allocated on the
stack. I don't think atomic_long_t behaves differently.

> --> On the other hand, if a variable of type work_struct is dynamically allocated, an unnecessary warning may be generated randomly; however, it should be a good habit to initialize dynamically allocated variables to 0.

It seems surprising having to initialize a variable to 0 only to
directly afterwards initialize it again with INIT_DELAYED_WORK().

Also even if there was consensus that it should be done this way,
existing users of this macro don't do it, so these would need to be
adapted first.

> >                 INIT_WORK(&(_work)->work, (_func));                     \
> >                 __init_timer(&(_work)->timer,                           \
> >                              delayed_work_timer_fn,                     \
> > @@ -318,6 +335,7 @@ static inline unsigned int work_static(struct
> > work_struct *work) { return 0; }
> 
> [..]
> 
> > --
> > 2.17.1
> >
> > ________________________________
> > OPPO
> 
> The gunk at the end of the mail will prevent your patch from being considered at all.
> 
> >
> > ±¾µç×ÓÓÊ¼þ¼°Æä¸½¼þº¬ÓÐOPPO¹«Ë¾µÄ±£ÃÜÐÅÏ¢£¬½öÏÞÓÚÓÊ¼þÖ¸Ã÷µÄÊÕ¼þÈË£¨°üº¬
> > ¸öÈË¼°Èº×é£©Ê¹ÓÃ¡£½ûÖ¹ÈÎºÎÈËÔÚÎ´¾ÊÚÈ¨µÄÇé¿öÏÂÒÔÈÎºÎÐÎÊ½Ê¹ÓÃ¡£Èç¹ûÄú´í
> > ÊÕÁË±¾ÓÊ¼þ£¬ÇÐÎð´«²¥¡¢·Ö·¢¡¢¸´ÖÆ¡¢Ó¡Ë¢»òÊ¹ÓÃ±¾ÓÊ¼þÖ®ÈÎºÎ²¿·Ö»òÆäËùÔØÖ®
> > ÈÎºÎÄÚÈÝ£¬²¢ÇëÁ¢¼´ÒÔµç×ÓÓÊ¼þÍ¨Öª·¢¼þÈË²¢É¾³ý±¾ÓÊ¼þ¼°Æä¸½¼þ¡£

This stuff really needs to go.