Re: [PATCH 5/7] zsmalloc: embed struct zs_handle into zram and zswap entries

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/11/05 17:19), Yosry Ahmed wrote:
[..]
> >  Additionally, zs_malloc() does not return a dereferenceable pointer.
> > -Instead, it returns an opaque handle (unsigned long) which encodes actual
> > -location of the allocated object. The reason for this indirection is that
> > +Instead, it stores the encoded location of the allocated object in the
> > +provided ``struct zs_handle``. The reason for this indirection is that
> 
> Should we explicitly call out that zsmalloc will keep a back pointer to
> the handle and update it asynchronously, so the owner shouldn't modify
> it, and should keep it allocated until zs_free() (or rebind)?
> 
> Probably somewhere in the code comments too?

Yeah, perhaps.

> >  zsmalloc does not keep zspages permanently mapped since that would cause
> >  issues on 32-bit systems where the VA region for kernel space mappings
> >  is very small. So, using the allocated memory should be done through the
> > diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> > index 693c9484b638..6c76fd10344e 100644
> > --- a/include/linux/zsmalloc.h
> > +++ b/include/linux/zsmalloc.h
> > @@ -14,6 +14,7 @@
> >  #ifndef _ZS_MALLOC_H_
> >  #define _ZS_MALLOC_H_
> >  
> > +#include <linux/compiler.h>
> >  #include <linux/types.h>
> >  
> >  struct zs_pool_stats {
> > @@ -21,16 +22,39 @@ struct zs_pool_stats {
> >  	atomic_long_t pages_compacted;
> >  };
> >  
> > +/*
> > + * zsmalloc writes object location into the handle and updates it when
> > + * the object moves (compaction, page migration), so the handle must not
> > + * be on the stack and must not be copied.  Use zs_rebind_handle() to
> > + * move an object to another handle.
> > + */
> > +struct zs_handle {
> > +	unsigned long __private obj;
> > +} __aligned(sizeof(unsigned long));
> 
> Is alignment guaranteed even if this is embedded in a packed struct? I
> wonder if we should have an explicit alignment check in
> zs_malloc()/zs_store(), and probably rebind too?

I guess only run-time check will work.  I can add those.  I wonder
if we actually need to worry about this, neither zram nor zswap use
packed entries, but I think it's cheap enough to make sure in run-time.

> > +
> > +static inline void zs_handle_init(struct zs_handle *handle)
> > +{
> > +	ACCESS_PRIVATE(handle, obj) = 0;
> > +}
> > +
> > +static inline bool zs_handle_is_allocated(const struct zs_handle *handle)
> > +{
> > +	return READ_ONCE(ACCESS_PRIVATE(handle, obj)) != 0;
> > +}
> 
> Can this ever give a false negative?

That's a very good question.

> I believe that's only possible with pfn=0, I don't know if this can be the
> case on any architecture. Should we use something like OBJ_ALLOCATED_TAG?

Yeah, I don't know, I guess would be better to not rely on "pfn cannot be
0".  Let me look into this.


> > -struct zs_handle *zs_malloc(struct zs_pool *pool, size_t size, gfp_t flags,
> > -			    const int nid);
> > +int zs_malloc(struct zs_pool *pool, struct zs_handle *handle, size_t size,
> > +	      gfp_t flags, const int nid);
> >  void zs_free(struct zs_pool *pool, struct zs_handle *handle);
> > +#ifdef CONFIG_ZSMALLOC_REBIND_HANDLE
> 
> FWIW, I don't think this is buying as much. It's one function that is
> only called by zram anyway.

Ack.

> > +void zs_rebind_handle(struct zs_pool *pool, struct zs_handle *dst,
> > +		      struct zs_handle *src);
> 
> A few comments in zsmalloc.c mention zs_free() reading the handle
> locklessly, do these apply to rebinding as well?

Let me take a look.

> > +#endif
> >  
> >  size_t zs_huge_class_size(struct zs_pool *pool);
> 
> [..]
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 397b06c9f33b..f28a46cae1e1 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -215,7 +215,7 @@ struct zswap_entry {
> >  	unsigned int length;
> >  	bool referenced;
> >  	u8 pool_idx;
> > -	struct zs_handle *handle;
> > +	struct zs_handle handle;
> >  	struct obj_cgroup *objcg;
> >  	struct list_head lru;
> >  };
> > @@ -769,6 +769,7 @@ static struct zswap_entry *zswap_entry_cache_alloc(gfp_t gfp, int nid)
> >  	entry = kmem_cache_alloc_node(zswap_entry_cache, gfp, nid);
> >  	if (!entry)
> >  		return NULL;
> > +	zs_handle_init(&entry->handle);
> >  	return entry;
> >  }
> >  
> > @@ -787,7 +788,7 @@ static void zswap_entry_free(struct zswap_entry *entry)
> >  
> >  	zswap_lru_del(entry);
> >  	if (!WARN_ON_ONCE(!pool)) {
> 
> This check is mostly defensive, and if the pool was ever NULL it
> prevented a NULL deref, and potentially leaked the underlying zsmalloc
> object, which is mostly fine as the pool being NULL points to a larger
> problem (e.g. corruption).
> 
> However, if that happens now, the handle pointer in zsmalloc would point
> at the zswap entry after it's freed by zswap_entry_cache_free() below,
> potentially causing a UAF. It's arguably fine as it's just a cascade of
> corruptions, but I wonder if we should just bail early and skip all
> cleanups below if the pool is NULL.
> 
> We'll leak the cgroup charge, stats, and the zswap entry itself -- but
> that's arguably the safer option if we already have a corruption
> problem.
> 
> We can also just delete the NULL check, but it's cheap to keep and I'd
> rather have it tbh.

Sounds good, let me reshuffle the code a bit.