[kernel-hardening] atomic64_wrap_t generic implementation

[kernel-hardening] atomic64_wrap_t generic implementation

[Colin Vidal]

Hi Elena,

diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
index dad68bf..4987419 100644
--- a/include/asm-generic/atomic64.h
+++ b/include/asm-generic/atomic64.h
@@ -16,6 +16,8 @@ typedef struct {
        long long counter;
 } atomic64_t;
 
+typedef atomic64_t atomic64_wrap_t;
+
 #define ATOMIC64_INIT(i)       { (i) }
 
 extern long long atomic64_read(const atomic64_t *v);
@@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
 #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
 #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
 
+#define atomic64_read_wrap(v) atomic64_read(v)
+#define atomic64_set_wrap(v, i) atomic64_set((v), (i))
+#define atomic64_add_wrap(a, v) atomic64_add((a), (v))
+#define atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v))
+#define atomic64_sub_wrap(a, v) atomic64_sub((a), (v))
+#define atomic64_inc_wrap(v) atomic64_inc(v)
+#define atomic64_inc_return_wrap(v) atomic64_inc_return(v)
+#define atomic64_dec_wrap(v) atomic64_dec(v)
+#define atomic64_cmpxchg_wrap(v, o, n) atomic64_cmpxchg((v), (o), (n))
+#define atomic64_xchg_wrap(v, n) atomic64_xchg((v), (n))

Isen't there a type error ? For instance:

atomic64_wrap_t atom_wrap;

atomic64_read_wrap(atom_wrap) will be expanded into
atomic64_read(atom_wrap), which would lead to a type error (atomic64_t
expected).

Perhaps the more simple thing to do would be

-typedef atomic64_t atomic64_wrap_t;
+#define atomic64_wrap_t atomic64_t

since the implementations are the same here.

Or I missed something obvious?

Thanks,

Colin

[kernel-hardening] RE: atomic64_wrap_t generic implementation

[Reshetova, Elena]

Hi Colin, 

diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h index dad68bf..4987419 100644
--- a/include/asm-generic/atomic64.h
+++ b/include/asm-generic/atomic64.h
@@ -16,6 +16,8 @@ typedef struct {
        long long counter;
 } atomic64_t;
 
+typedef atomic64_t atomic64_wrap_t;
+
 #define ATOMIC64_INIT(i)       { (i) }
 
 extern long long atomic64_read(const atomic64_t *v); @@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
 #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
 #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
 
+#define atomic64_read_wrap(v) atomic64_read(v) #define 
+atomic64_set_wrap(v, i) atomic64_set((v), (i)) #define 
+atomic64_add_wrap(a, v) atomic64_add((a), (v)) #define 
+atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v)) #define 
+atomic64_sub_wrap(a, v) atomic64_sub((a), (v)) #define 
+atomic64_inc_wrap(v) atomic64_inc(v) #define 
+atomic64_inc_return_wrap(v) atomic64_inc_return(v) #define 
+atomic64_dec_wrap(v) atomic64_dec(v) #define atomic64_cmpxchg_wrap(v, 
+o, n) atomic64_cmpxchg((v), (o), (n)) #define atomic64_xchg_wrap(v, n) 
+atomic64_xchg((v), (n))

>Isen't there a type error ? For instance:
>atomic64_wrap_t atom_wrap;
>atomic64_read_wrap(atom_wrap) will be expanded into atomic64_read(atom_wrap), which would lead to a type error (atomic64_t expected).

I have been double checking this now and I think for x86, all the changes in asm-generic/atomic64.h are not needed. 
X86 overrides them with arch. specific implementations in include/asm/atomic64_64.h and include/asm/atomic64_32.h

Takahiro, you have been looking into arm. Are the above ones used in any way in arm or can we simply get rid of the whole bundle of changes?

Best Regards,
Elena.

[kernel-hardening] Re: atomic64_wrap_t generic implementation

[Colin Vidal]

Hi Elena,

On Thu, 2016-10-13 at 17:25 +0000, Reshetova, Elena wrote:
> Hi Colin, 
> 
> diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h index dad68bf..4987419 100644
> --- a/include/asm-generic/atomic64.h
> +++ b/include/asm-generic/atomic64.h
> @@ -16,6 +16,8 @@ typedef struct {
>         long long counter;
>  } atomic64_t;
>  
> +typedef atomic64_t atomic64_wrap_t;
> +
>  #define ATOMIC64_INIT(i)       { (i) }
>  
>  extern long long atomic64_read(const atomic64_t *v); @@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
>  #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
>  #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
>  
> +#define atomic64_read_wrap(v) atomic64_read(v) #define 
> +atomic64_set_wrap(v, i) atomic64_set((v), (i)) #define 
> +atomic64_add_wrap(a, v) atomic64_add((a), (v)) #define 
> +atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v)) #define 
> +atomic64_sub_wrap(a, v) atomic64_sub((a), (v)) #define 
> +atomic64_inc_wrap(v) atomic64_inc(v) #define 
> +atomic64_inc_return_wrap(v) atomic64_inc_return(v) #define 
> +atomic64_dec_wrap(v) atomic64_dec(v) #define atomic64_cmpxchg_wrap(v, 
> +o, n) atomic64_cmpxchg((v), (o), (n)) #define atomic64_xchg_wrap(v, n) 
> +atomic64_xchg((v), (n))
> 
> > 
> > Isen't there a type error ? For instance:
> > atomic64_wrap_t atom_wrap;
> > atomic64_read_wrap(atom_wrap) will be expanded into atomic64_read(atom_wrap), which would lead to a type error (atomic64_t expected).
> 
> I have been double checking this now and I think for x86, all the changes in asm-generic/atomic64.h are not needed. 
> X86 overrides them with arch. specific implementations in include/asm/atomic64_64.h and include/asm/atomic64_32.h
> 

Arghhh sorry, I was confused about the type error I was talking about,
of course the previous code obviously correct...

> Takahiro, you have been looking into arm. Are the above ones used in any way in arm or can we simply get rid of the whole bundle of changes?
> 

I can't tell for ARM64, but I note that ARM has its own implementation
of atomic64 only if CONFIG_GENERIC_ATOMIC64 is unset. It can be set
according to arch/arm/Kconfig:

select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)

Anyways, some architectures uses asm-generic/atomic64.h without guards
(metatag, microblaze, sparc32), so I supposed it is mandatory to keep
those macros.

Best regards,

Colin

Re: [kernel-hardening] RE: atomic64_wrap_t generic implementation

[David Windsor]

On Thu, Oct 13, 2016 at 1:25 PM, Reshetova, Elena
<elena.reshetova@intel.com> wrote:
> Hi Colin,
>
> diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h index dad68bf..4987419 100644
> --- a/include/asm-generic/atomic64.h
> +++ b/include/asm-generic/atomic64.h
> @@ -16,6 +16,8 @@ typedef struct {
>         long long counter;
>  } atomic64_t;
>
> +typedef atomic64_t atomic64_wrap_t;
> +
>  #define ATOMIC64_INIT(i)       { (i) }
>
>  extern long long atomic64_read(const atomic64_t *v); @@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
>  #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
>  #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
>
> +#define atomic64_read_wrap(v) atomic64_read(v) #define
> +atomic64_set_wrap(v, i) atomic64_set((v), (i)) #define
> +atomic64_add_wrap(a, v) atomic64_add((a), (v)) #define
> +atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v)) #define
> +atomic64_sub_wrap(a, v) atomic64_sub((a), (v)) #define
> +atomic64_inc_wrap(v) atomic64_inc(v) #define
> +atomic64_inc_return_wrap(v) atomic64_inc_return(v) #define
> +atomic64_dec_wrap(v) atomic64_dec(v) #define atomic64_cmpxchg_wrap(v,
> +o, n) atomic64_cmpxchg((v), (o), (n)) #define atomic64_xchg_wrap(v, n)
> +atomic64_xchg((v), (n))
>
>>Isen't there a type error ? For instance:
>>atomic64_wrap_t atom_wrap;
>>atomic64_read_wrap(atom_wrap) will be expanded into atomic64_read(atom_wrap), which would lead to a type error (atomic64_t expected).
>
> I have been double checking this now and I think for x86, all the changes in asm-generic/atomic64.h are not needed.
> X86 overrides them with arch. specific implementations in include/asm/atomic64_64.h and include/asm/atomic64_32.h
>
> Takahiro, you have been looking into arm. Are the above ones used in any way in arm or can we simply get rid of the whole bundle of changes?
>

I think it best to leave the definitions in asm-generic/atomic64.h.
While not used on x86, there are other architectures lacking 64-bit
atomic operations that need this definition of atomic64_t (and its
associated implementation with hashed spinlocks): this original
feature was added in support of the perf_counter subsystem [1]; yet
another perf-related exception.  If we want the support of
HARDENED_ATOMIC to extend to these other architectures, I think we
should leave this in there.

[1] https://lwn.net/Articles/337534/

> Best Regards,
> Elena.
>
>

RE: [kernel-hardening] RE: atomic64_wrap_t generic implementation

[Reshetova, Elena]

>I think it best to leave the definitions in asm-generic/atomic64.h.
>While not used on x86, there are other architectures lacking 64-bit atomic operations that need this definition of atomic64_t (and its associated implementation with hashed spinlocks): this original feature was added in support of the perf_counter >subsystem [1]; yet another perf-related exception.  If we want the support of HARDENED_ATOMIC to extend to these other architectures, I think we should leave this in there.

Fair point.  So, I let them live where they are now. 

Best Regards,
Elena

Re: [kernel-hardening] atomic64_wrap_t generic implementation

[AKASHI Takahiro]

On Wed, Oct 12, 2016 at 09:42:12PM +0200, Colin Vidal wrote:
> Hi Elena,
> 
> diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
> index dad68bf..4987419 100644
> --- a/include/asm-generic/atomic64.h
> +++ b/include/asm-generic/atomic64.h
> @@ -16,6 +16,8 @@ typedef struct {
>         long long counter;
>  } atomic64_t;
>  
> +typedef atomic64_t atomic64_wrap_t;
> +
>  #define ATOMIC64_INIT(i)       { (i) }
>  
>  extern long long atomic64_read(const atomic64_t *v);
> @@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
>  #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
>  #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
>  
> +#define atomic64_read_wrap(v) atomic64_read(v)
> +#define atomic64_set_wrap(v, i) atomic64_set((v), (i))
> +#define atomic64_add_wrap(a, v) atomic64_add((a), (v))
> +#define atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v))
> +#define atomic64_sub_wrap(a, v) atomic64_sub((a), (v))
> +#define atomic64_inc_wrap(v) atomic64_inc(v)
> +#define atomic64_inc_return_wrap(v) atomic64_inc_return(v)
> +#define atomic64_dec_wrap(v) atomic64_dec(v)
> +#define atomic64_cmpxchg_wrap(v, o, n) atomic64_cmpxchg((v), (o), (n))
> +#define atomic64_xchg_wrap(v, n) atomic64_xchg((v), (n))
> 
> Isen't there a type error ? For instance:
> 
> atomic64_wrap_t atom_wrap;
> 
> atomic64_read_wrap(atom_wrap) will be expanded into
> atomic64_read(atom_wrap), which would lead to a type error (atomic64_t
> expected).
> 
> Perhaps the more simple thing to do would be
> 
> -typedef atomic64_t atomic64_wrap_t;

As atomic64_wrap_t is always defined in include/linux/types.h
(under CONFIG_64BIT), this definition should not be here.

> +#define atomic64_wrap_t atomic64_t

and it would be better off to always define atomic(64)_wrap_t as a struct
in order to detect potential misusages like:
    atomic64_t atom_var;
    atomic64_read_wrap(&atom_var);
?

Thanks,
-Takahiro AKASHI

> since the implementations are the same here.
> 
> Or I missed something obvious?
> 
> Thanks,
> 
> Colin

Re: [kernel-hardening] atomic64_wrap_t generic implementation

[Colin Vidal]

On Tue, 2016-10-18 at 17:04 +0900, AKASHI Takahiro wrote:
> On Wed, Oct 12, 2016 at 09:42:12PM +0200, Colin Vidal wrote:
> > 
> > Hi Elena,
> > 
> > diff --git a/include/asm-generic/atomic64.h b/include/asm-generic/atomic64.h
> > index dad68bf..4987419 100644
> > --- a/include/asm-generic/atomic64.h
> > +++ b/include/asm-generic/atomic64.h
> > @@ -16,6 +16,8 @@ typedef struct {
> >         long long counter;
> >  } atomic64_t;
> >  
> > +typedef atomic64_t atomic64_wrap_t;
> > +
> >  #define ATOMIC64_INIT(i)       { (i) }
> >  
> >  extern long long atomic64_read(const atomic64_t *v);
> > @@ -62,4 +64,15 @@ extern int    atomic64_add_unless(atomic64_t *v, long long a, long long u);
> >  #define atomic64_dec_and_test(v)       (atomic64_dec_return((v)) == 0)
> >  #define atomic64_inc_not_zero(v)       atomic64_add_unless((v), 1LL, 0LL)
> >  
> > +#define atomic64_read_wrap(v) atomic64_read(v)
> > +#define atomic64_set_wrap(v, i) atomic64_set((v), (i))
> > +#define atomic64_add_wrap(a, v) atomic64_add((a), (v))
> > +#define atomic64_add_return_wrap(a, v) atomic64_add_return((a), (v))
> > +#define atomic64_sub_wrap(a, v) atomic64_sub((a), (v))
> > +#define atomic64_inc_wrap(v) atomic64_inc(v)
> > +#define atomic64_inc_return_wrap(v) atomic64_inc_return(v)
> > +#define atomic64_dec_wrap(v) atomic64_dec(v)
> > +#define atomic64_cmpxchg_wrap(v, o, n) atomic64_cmpxchg((v), (o), (n))
> > +#define atomic64_xchg_wrap(v, n) atomic64_xchg((v), (n))
> > 
> > Isen't there a type error ? For instance:
> > 
> > atomic64_wrap_t atom_wrap;
> > 
> > atomic64_read_wrap(atom_wrap) will be expanded into
> > atomic64_read(atom_wrap), which would lead to a type error (atomic64_t
> > expected).
> > 
> > Perhaps the more simple thing to do would be
> > 
> > -typedef atomic64_t atomic64_wrap_t;
> 
> As atomic64_wrap_t is always defined in include/linux/types.h
> (under CONFIG_64BIT), this definition should not be here.

Yep, my mistake, sorry. I missed a part of include/linux/types.h when I
wrote that previous mail.

> > +#define atomic64_wrap_t atomic64_t
> 
> and it would be better off to always define atomic(64)_wrap_t as a struct
> in order to detect potential misusages like:
>     atomic64_t atom_var;
>     atomic64_read_wrap(&atom_var);
> ?

I agree. Still, it would need some modifications to cast atomic*_wrap_t
to atomic*_t into whole macros that wrap atomic*_wrap to atomic*
functions in asm-generic/{atomic64.h,atomic-long.h}.

Thanks,

Colin