linux-kernel - Re: [PATCH v2 1/7] sched/fair: Provide u64 read for 32-bits arch helper

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]

Date:   Tue, 18 Jan 2022 07:44:54 +0800
From:   Tao Zhou <tao.zhou@...ux.dev>
To:     Vincent Donnefort <vincent.donnefort@....com>
Cc:     peterz@...radead.org, mingo@...hat.com, vincent.guittot@...aro.org,
        linux-kernel@...r.kernel.org, dietmar.eggemann@....com,
        valentin.schneider@....com, morten.rasmussen@....com,
        chris.redpath@....com, qperret@...gle.com, lukasz.luba@....com,
        Tao Zhou <tao.zhou@...ux.dev>
Subject: Re: [PATCH v2 1/7] sched/fair: Provide u64 read for 32-bits arch
 helper

On Mon, Jan 17, 2022 at 07:42:04PM +0000, Vincent Donnefort wrote:
> [...]
> 
> > > +/*
> > > + * u64_u32_load/u64_u32_store
> > > + *
> > > + * Use a copy of a u64 value to protect against data race. This is only
> > > + * applicable for 32-bits architectures.
> > > + */
> > > +#ifdef CONFIG_64BIT
> > > +# define u64_u32_load_copy(var, copy)       var
> > > +# define u64_u32_store_copy(var, copy, val) (var = val)
> > > +#else
> > > +# define u64_u32_load_copy(var, copy)					\
> > > +({									\
> > > +	u64 __val, __val_copy;						\
> > > +	do {								\
> > > +		__val_copy = copy;					\
> > > +		/*							\
> > > +		 * paired with u64_u32_store, ordering access		\
> > > +		 * to var and copy.					\
> > > +		 */							\
> > > +		smp_rmb();						\
> > > +		__val = var;						\
> > > +	} while (__val != __val_copy);					\
> > > +	__val;								\
> > > +})
> > > +# define u64_u32_store_copy(var, copy, val)				\
> > > +do {									\
> > > +	typeof(val) __val = (val);					\
> > > +	var = __val;							\
> > > +	/*								\
> > > +	 * paired with u64_u32_load, ordering access to var and		\
> > > +	 * copy.							\
> > > +	 */								\
> > > +	smp_wmb();							\
> > > +	copy = __val;							\
> > > +} while (0)
> > 
> > Code stay there some time from me. Just from my crude review;
> > The above macro need a variable to load @var temporarily for
> > later store; that means the @copy value is from @var not @val.
> > 
> >   # define u64_u32_store_copy(var, copy, val)				\
> >   do {									\
> >     typeof(val) __val = (val), __var = (var);					\
> >     var = __val;							\
> >     /*								\
> >      * paired with u64_u32_load, ordering access to var and		\
> >      * copy.							\
> >      */								\
> >     smp_wmb();							\
> >     copy = __var;							\
> >   } while (0)
> 
> 
> Hi Tao,
> 
> __var would then contain the previous value of @var, wouldn't it? We need
> both @var and @copy to be equal to @val.

Sorry for the noise, I'm wrong here.

> 
> > 
> > 
> > 
> > Thanks,
> > Tao