| Message ID | 20201216231550.27224-5-rpearson@hpe.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: cleanup and extensions | expand |
On Wed, Dec 16, 2020 at 05:15:47PM -0600, Bob Pearson wrote: > The allocate, lookup index, lookup key and cleanup routines > in rxe_pool.c currently are not type safe against relocating > the pelem field in the objects. Planned changes to move > allocation of objects into rdma-core make addressing this a > requirement. > > Use the elem_offset field in rxe_type_info make these APIs > safe against moving the pelem field. > > Signed-off-by: Bob Pearson <rpearson@hpe.com> > drivers/infiniband/sw/rxe/rxe_pool.c | 55 +++++++++++++++++++--------- > 1 file changed, 38 insertions(+), 17 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c > index 4d667b78af9b..2873ecfb84c2 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > @@ -315,7 +315,9 @@ void rxe_drop_index(void *arg) > > void *rxe_alloc(struct rxe_pool *pool) > { > + struct rxe_type_info *info = &rxe_type_info[pool->type]; > struct rxe_pool_entry *elem; > + u8 *obj; > unsigned long flags; > > might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); > @@ -334,16 +336,17 @@ void *rxe_alloc(struct rxe_pool *pool) > if (atomic_inc_return(&pool->num_elem) > pool->max_elem) > goto out_cnt; > > - elem = kzalloc(rxe_type_info[pool->type].size, > - (pool->flags & RXE_POOL_ATOMIC) ? > - GFP_ATOMIC : GFP_KERNEL); > - if (!elem) > + obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ? > + GFP_ATOMIC : GFP_KERNEL); > + if (!obj) > goto out_cnt; > > + elem = (struct rxe_pool_entry *)(obj + info->elem_offset); This makes more sense squashed with the https://patchwork.kernel.org/project/linux-rdma/patch/20201216231550.27224-4-rpearson@hpe.com/ patch But I would suggest a simpler answer, the pool APIs should always work with 'struct rxe_pool_entry *' Here it should return it: struct rxe_pool_entry *_rxe_alloc(struct rxe_pool *pool, size_t size) And then use a compile time macro to enforce that pelm is always first: #define rxe_alloc(pool, type) \ container_of(_rxe_alloc(pool, sizeof(type) + BUILD_BUG_ON(offsetof(type, pelem) != 0)), type, pelm) This would eliminate all the ugly void *'s from the API. Just the allocator needs the BUILD_BUG_ON, but all the other places really ought to use container_of() not void * to transform the rxe_pool_entry to its larger struct. Also I noticed this when I was looking - which also means size can be removed from the struct rxe_type_info as the allocation here was the only use. diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index d26730eec720c6..d515314510ed6a 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -128,14 +128,12 @@ int rxe_pool_init( unsigned int max_elem) { int err = 0; - size_t size = rxe_type_info[type].size; memset(pool, 0, sizeof(*pool)); pool->rxe = rxe; pool->type = type; pool->max_elem = max_elem; - pool->elem_size = ALIGN(size, RXE_POOL_ALIGN); pool->flags = rxe_type_info[type].flags; pool->index.tree = RB_ROOT; pool->key.tree = RB_ROOT; diff --git a/drivers/infiniband/sw/rxe/rxe_pool.h b/drivers/infiniband/sw/rxe/rxe_pool.h index 373e08554c1c9d..f34da4d33e243b 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.h +++ b/drivers/infiniband/sw/rxe/rxe_pool.h @@ -68,7 +68,6 @@ struct rxe_pool_entry { struct rxe_pool { struct rxe_dev *rxe; rwlock_t pool_lock; /* protects pool add/del/search */ - size_t elem_size; struct kref ref_cnt; void (*cleanup)(struct rxe_pool_entry *obj); enum rxe_pool_state state;
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index 4d667b78af9b..2873ecfb84c2 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -315,7 +315,9 @@ void rxe_drop_index(void *arg) void *rxe_alloc(struct rxe_pool *pool) { + struct rxe_type_info *info = &rxe_type_info[pool->type]; struct rxe_pool_entry *elem; + u8 *obj; unsigned long flags; might_sleep_if(!(pool->flags & RXE_POOL_ATOMIC)); @@ -334,16 +336,17 @@ void *rxe_alloc(struct rxe_pool *pool) if (atomic_inc_return(&pool->num_elem) > pool->max_elem) goto out_cnt; - elem = kzalloc(rxe_type_info[pool->type].size, - (pool->flags & RXE_POOL_ATOMIC) ? - GFP_ATOMIC : GFP_KERNEL); - if (!elem) + obj = kzalloc(info->size, (pool->flags & RXE_POOL_ATOMIC) ? + GFP_ATOMIC : GFP_KERNEL); + if (!obj) goto out_cnt; + elem = (struct rxe_pool_entry *)(obj + info->elem_offset); + elem->pool = pool; kref_init(&elem->ref_cnt); - return elem; + return obj; out_cnt: atomic_dec(&pool->num_elem); @@ -391,12 +394,17 @@ void rxe_elem_release(struct kref *kref) struct rxe_pool_entry *elem = container_of(kref, struct rxe_pool_entry, ref_cnt); struct rxe_pool *pool = elem->pool; + struct rxe_type_info *info = &rxe_type_info[pool->type]; + u8 *obj; if (pool->cleanup) pool->cleanup(elem); - if (!(pool->flags & RXE_POOL_NO_ALLOC)) - kfree(elem); + if (!(pool->flags & RXE_POOL_NO_ALLOC)) { + obj = (u8 *)elem - info->elem_offset; + kfree(obj); + } + atomic_dec(&pool->num_elem); ib_device_put(&pool->rxe->ib_dev); rxe_pool_put(pool); @@ -404,8 +412,10 @@ void rxe_elem_release(struct kref *kref) void *rxe_pool_get_index(struct rxe_pool *pool, u32 index) { - struct rb_node *node = NULL; - struct rxe_pool_entry *elem = NULL; + struct rxe_type_info *info = &rxe_type_info[pool->type]; + struct rb_node *node; + struct rxe_pool_entry *elem; + u8 *obj = NULL; unsigned long flags; read_lock_irqsave(&pool->pool_lock, flags); @@ -422,21 +432,28 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index) node = node->rb_left; else if (elem->index < index) node = node->rb_right; - else { - kref_get(&elem->ref_cnt); + else break; - } + } + + if (node) { + kref_get(&elem->ref_cnt); + obj = (u8 *)elem - info->elem_offset; + } else { + obj = NULL; } out: read_unlock_irqrestore(&pool->pool_lock, flags); - return node ? elem : NULL; + return obj; } void *rxe_pool_get_key(struct rxe_pool *pool, void *key) { - struct rb_node *node = NULL; - struct rxe_pool_entry *elem = NULL; + struct rxe_type_info *info = &rxe_type_info[pool->type]; + struct rb_node *node; + struct rxe_pool_entry *elem; + u8 *obj = NULL; int cmp; unsigned long flags; @@ -461,10 +478,14 @@ void *rxe_pool_get_key(struct rxe_pool *pool, void *key) break; } - if (node) + if (node) { kref_get(&elem->ref_cnt); + obj = (u8 *)elem - info->elem_offset; + } else { + obj = NULL; + } out: read_unlock_irqrestore(&pool->pool_lock, flags); - return node ? elem : NULL; + return obj; }
The allocate, lookup index, lookup key and cleanup routines in rxe_pool.c currently are not type safe against relocating the pelem field in the objects. Planned changes to move allocation of objects into rdma-core make addressing this a requirement. Use the elem_offset field in rxe_type_info make these APIs safe against moving the pelem field. Signed-off-by: Bob Pearson <rpearson@hpe.com> --- drivers/infiniband/sw/rxe/rxe_pool.c | 55 +++++++++++++++++++--------- 1 file changed, 38 insertions(+), 17 deletions(-)