[PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[Breno Leitao]

Having cache entries linked using the hlist format brings no benefit, and
also requires an unnecessary extra pointer address per cache entry.

Use the internal io_wq_work_node single-linked list for the internal
alloc caches (async_msghdr and async_poll)

This is required to be able to use KASAN on cache entries, since we do
not need to touch unused (and poisoned) cache entries when adding more
entries to the list.

Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 include/linux/io_uring_types.h |  2 +-
 io_uring/alloc_cache.h         | 27 +++++++++++++++------------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index 0efe4d784358..efa66b6c32c9 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -188,7 +188,7 @@ struct io_ev_fd {
 };
 
 struct io_alloc_cache {
-	struct hlist_head	list;
+	struct io_wq_work_node	list;
 	unsigned int		nr_cached;
 };
 
diff --git a/io_uring/alloc_cache.h b/io_uring/alloc_cache.h
index 729793ae9712..0d9ff9402a37 100644
--- a/io_uring/alloc_cache.h
+++ b/io_uring/alloc_cache.h
@@ -7,7 +7,7 @@
 #define IO_ALLOC_CACHE_MAX	512
 
 struct io_cache_entry {
-	struct hlist_node	node;
+	struct io_wq_work_node node;
 };
 
 static inline bool io_alloc_cache_put(struct io_alloc_cache *cache,
@@ -15,7 +15,7 @@ static inline bool io_alloc_cache_put(struct io_alloc_cache *cache,
 {
 	if (cache->nr_cached < IO_ALLOC_CACHE_MAX) {
 		cache->nr_cached++;
-		hlist_add_head(&entry->node, &cache->list);
+		wq_stack_add_head(&entry->node, &cache->list);
 		return true;
 	}
 	return false;
@@ -23,11 +23,14 @@ static inline bool io_alloc_cache_put(struct io_alloc_cache *cache,
 
 static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *cache)
 {
-	if (!hlist_empty(&cache->list)) {
-		struct hlist_node *node = cache->list.first;
-
-		hlist_del(node);
-		return container_of(node, struct io_cache_entry, node);
+	struct io_wq_work_node *node;
+	struct io_cache_entry *entry;
+
+	if (cache->list.next) {
+		node = cache->list.next;
+		entry = container_of(node, struct io_cache_entry, node);
+		cache->list.next = node->next;
+		return entry;
 	}
 
 	return NULL;
@@ -35,19 +38,19 @@ static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *c
 
 static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
 {
-	INIT_HLIST_HEAD(&cache->list);
+	cache->list.next = NULL;
 	cache->nr_cached = 0;
 }
 
 static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
 					void (*free)(struct io_cache_entry *))
 {
-	while (!hlist_empty(&cache->list)) {
-		struct hlist_node *node = cache->list.first;
+	struct io_cache_entry *entry;
 
-		hlist_del(node);
-		free(container_of(node, struct io_cache_entry, node));
+	while ((entry = io_alloc_cache_get(cache))) {
+		free(entry);
 	}
+
 	cache->nr_cached = 0;
 }
 #endif
-- 
2.39.0

[PATCH 2/2] io_uring: Add KASAN support for alloc_caches

[Breno Leitao]

Add support for KASAN in the alloc_caches (apoll and netmsg_cache).
Thus, if something touches the unused caches, it will raise a KASAN
warning/exception.

It poisons the object when the object is put to the cache, and unpoisons
it when the object is gotten or freed.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 io_uring/alloc_cache.h | 11 ++++++++---
 io_uring/io_uring.c    | 12 ++++++++++--
 io_uring/net.c         |  2 +-
 io_uring/poll.c        |  2 +-
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/io_uring/alloc_cache.h b/io_uring/alloc_cache.h
index 0d9ff9402a37..0d5cd2c0a0ba 100644
--- a/io_uring/alloc_cache.h
+++ b/io_uring/alloc_cache.h
@@ -16,12 +16,15 @@ static inline bool io_alloc_cache_put(struct io_alloc_cache *cache,
 	if (cache->nr_cached < IO_ALLOC_CACHE_MAX) {
 		cache->nr_cached++;
 		wq_stack_add_head(&entry->node, &cache->list);
+		/* KASAN poisons object */
+		kasan_slab_free_mempool(entry);
 		return true;
 	}
 	return false;
 }
 
-static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *cache)
+static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *cache,
+							size_t size)
 {
 	struct io_wq_work_node *node;
 	struct io_cache_entry *entry;
@@ -29,6 +32,7 @@ static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *c
 	if (cache->list.next) {
 		node = cache->list.next;
 		entry = container_of(node, struct io_cache_entry, node);
+		kasan_unpoison_range(entry, size);
 		cache->list.next = node->next;
 		return entry;
 	}
@@ -43,11 +47,12 @@ static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
 }
 
 static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
-					void (*free)(struct io_cache_entry *))
+					void (*free)(struct io_cache_entry *),
+					size_t size)
 {
 	struct io_cache_entry *entry;
 
-	while ((entry = io_alloc_cache_get(cache))) {
+	while ((entry = io_alloc_cache_get(cache, size))) {
 		free(entry);
 	}
 
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 80b6204769e8..6a98902b8f62 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2766,6 +2766,15 @@ static void io_req_caches_free(struct io_ring_ctx *ctx)
 	mutex_unlock(&ctx->uring_lock);
 }
 
+static __cold void io_uring_acache_free(struct io_ring_ctx *ctx)
+{
+
+	io_alloc_cache_free(&ctx->apoll_cache, io_apoll_cache_free,
+			    sizeof(struct async_poll));
+	io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free,
+			    sizeof(struct io_async_msghdr));
+}
+
 static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
 {
 	io_sq_thread_finish(ctx);
@@ -2781,8 +2790,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
 		__io_sqe_files_unregister(ctx);
 	io_cqring_overflow_kill(ctx);
 	io_eventfd_unregister(ctx);
-	io_alloc_cache_free(&ctx->apoll_cache, io_apoll_cache_free);
-	io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free);
+	io_uring_acache_free(ctx);
 	mutex_unlock(&ctx->uring_lock);
 	io_destroy_buffers(ctx);
 	if (ctx->sq_creds)
diff --git a/io_uring/net.c b/io_uring/net.c
index fbc34a7c2743..8dc67b23b030 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -139,7 +139,7 @@ static struct io_async_msghdr *io_msg_alloc_async(struct io_kiocb *req,
 	struct io_async_msghdr *hdr;
 
 	if (!(issue_flags & IO_URING_F_UNLOCKED)) {
-		entry = io_alloc_cache_get(&ctx->netmsg_cache);
+		entry = io_alloc_cache_get(&ctx->netmsg_cache, sizeof(struct io_async_msghdr));
 		if (entry) {
 			hdr = container_of(entry, struct io_async_msghdr, cache);
 			hdr->free_iov = NULL;
diff --git a/io_uring/poll.c b/io_uring/poll.c
index 8339a92b4510..295d59875f00 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -661,7 +661,7 @@ static struct async_poll *io_req_alloc_apoll(struct io_kiocb *req,
 		apoll = req->apoll;
 		kfree(apoll->double_poll);
 	} else if (!(issue_flags & IO_URING_F_UNLOCKED)) {
-		entry = io_alloc_cache_get(&ctx->apoll_cache);
+		entry = io_alloc_cache_get(&ctx->apoll_cache, sizeof(struct async_poll));
 		if (entry == NULL)
 			goto alloc_apoll;
 		apoll = container_of(entry, struct async_poll, cache);
-- 
2.39.0

Re: [PATCH 2/2] io_uring: Add KASAN support for alloc_caches

[kernel test robot]

Hi Breno,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.2 next-20230221]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Breno-Leitao/io_uring-Add-KASAN-support-for-alloc_caches/20230221-220039
patch link:    https://lore.kernel.org/r/20230221135721.3230763-2-leitao%40debian.org
patch subject: [PATCH 2/2] io_uring: Add KASAN support for alloc_caches
config: powerpc-allnoconfig (https://download.01.org/0day-ci/archive/20230222/202302220015.B4dQkwgA-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d909e43a1659897df77bc1373d3c24cc0d9129cf
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Breno-Leitao/io_uring-Add-KASAN-support-for-alloc_caches/20230221-220039
        git checkout d909e43a1659897df77bc1373d3c24cc0d9129cf
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302220015.B4dQkwgA-lkp@intel.com/

All errors (new ones prefixed by >>):

   io_uring/io_uring.c: In function '__io_submit_flush_completions':
   io_uring/io_uring.c:1502:40: warning: variable 'prev' set but not used [-Wunused-but-set-variable]
    1502 |         struct io_wq_work_node *node, *prev;
         |                                        ^~~~
   io_uring/io_uring.c: In function 'io_uring_acache_free':
>> io_uring/io_uring.c:2781:36: error: invalid application of 'sizeof' to incomplete type 'struct io_async_msghdr'
    2781 |                             sizeof(struct io_async_msghdr));
         |                                    ^~~~~~


vim +2781 io_uring/io_uring.c

  2777	
  2778		io_alloc_cache_free(&ctx->apoll_cache, io_apoll_cache_free,
  2779				    sizeof(struct async_poll));
  2780		io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free,
> 2781				    sizeof(struct io_async_msghdr));
  2782	}
  2783	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[Pavel Begunkov]

On 2/21/23 13:57, Breno Leitao wrote:
> Having cache entries linked using the hlist format brings no benefit, and
> also requires an unnecessary extra pointer address per cache entry.
> 
> Use the internal io_wq_work_node single-linked list for the internal
> alloc caches (async_msghdr and async_poll)
> 
> This is required to be able to use KASAN on cache entries, since we do
> not need to touch unused (and poisoned) cache entries when adding more
> entries to the list.

Looks good, a few nits

> 
> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>   include/linux/io_uring_types.h |  2 +-
>   io_uring/alloc_cache.h         | 27 +++++++++++++++------------
>   2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index 0efe4d784358..efa66b6c32c9 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -188,7 +188,7 @@ struct io_ev_fd {
>   };
>   
[...]
> -	if (!hlist_empty(&cache->list)) {
> -		struct hlist_node *node = cache->list.first;
> -
> -		hlist_del(node);
> -		return container_of(node, struct io_cache_entry, node);
> +	struct io_wq_work_node *node;
> +	struct io_cache_entry *entry;
> +
> +	if (cache->list.next) {
> +		node = cache->list.next;
> +		entry = container_of(node, struct io_cache_entry, node);

I'd prefer to get rid of the node var, it'd be a bit cleaner
than keeping two pointers to the same chunk.

entry = container_of(node, struct io_cache_entry,
                      cache->list.next);

> +		cache->list.next = node->next;
> +		return entry;
>   	}
>   
>   	return NULL;
> @@ -35,19 +38,19 @@ static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *c
>   
>   static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
>   {
> -	INIT_HLIST_HEAD(&cache->list);
> +	cache->list.next = NULL;
>   	cache->nr_cached = 0;
>   }
>   
>   static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
>   					void (*free)(struct io_cache_entry *))
>   {
> -	while (!hlist_empty(&cache->list)) {
> -		struct hlist_node *node = cache->list.first;
> +	struct io_cache_entry *entry;
>   
> -		hlist_del(node);
> -		free(container_of(node, struct io_cache_entry, node));
> +	while ((entry = io_alloc_cache_get(cache))) {
> +		free(entry);

We don't need brackets here. Personally, I don't have anything
against assignments in if, but it's probably better to avoid them,
or there will be a patch in a couple of months based on a random
code analysis report as happened many times before.

while (1) {
	struct io_cache_entry *entry = get();

	if (!entry)
		break;
	free(entry);
}	

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[Breno Leitao]

On 21/02/2023 17:45, Pavel Begunkov wrote:
> On 2/21/23 13:57, Breno Leitao wrote:
>> Having cache entries linked using the hlist format brings no benefit, and
>> also requires an unnecessary extra pointer address per cache entry.
>>
>> Use the internal io_wq_work_node single-linked list for the internal
>> alloc caches (async_msghdr and async_poll)
>>
>> This is required to be able to use KASAN on cache entries, since we do
>> not need to touch unused (and poisoned) cache entries when adding more
>> entries to the list.
> 
> Looks good, a few nits
> 
>>
>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>>   include/linux/io_uring_types.h |  2 +-
>>   io_uring/alloc_cache.h         | 27 +++++++++++++++------------
>>   2 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/io_uring_types.h
>> b/include/linux/io_uring_types.h
>> index 0efe4d784358..efa66b6c32c9 100644
>> --- a/include/linux/io_uring_types.h
>> +++ b/include/linux/io_uring_types.h
>> @@ -188,7 +188,7 @@ struct io_ev_fd {
>>   };
>>   
> [...]
>> -    if (!hlist_empty(&cache->list)) {
>> -        struct hlist_node *node = cache->list.first;
>> -
>> -        hlist_del(node);
>> -        return container_of(node, struct io_cache_entry, node);
>> +    struct io_wq_work_node *node;
>> +    struct io_cache_entry *entry;
>> +
>> +    if (cache->list.next) {
>> +        node = cache->list.next;
>> +        entry = container_of(node, struct io_cache_entry, node);
> 
> I'd prefer to get rid of the node var, it'd be a bit cleaner
> than keeping two pointers to the same chunk.
> 
> entry = container_of(node, struct io_cache_entry,
>                      cache->list.next);
> 
>> +        cache->list.next = node->next;
>> +        return entry;
>>       }
>>         return NULL;
>> @@ -35,19 +38,19 @@ static inline struct io_cache_entry
>> *io_alloc_cache_get(struct io_alloc_cache *c
>>     static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
>>   {
>> -    INIT_HLIST_HEAD(&cache->list);
>> +    cache->list.next = NULL;
>>       cache->nr_cached = 0;
>>   }
>>     static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
>>                       void (*free)(struct io_cache_entry *))
>>   {
>> -    while (!hlist_empty(&cache->list)) {
>> -        struct hlist_node *node = cache->list.first;
>> +    struct io_cache_entry *entry;
>>   -        hlist_del(node);
>> -        free(container_of(node, struct io_cache_entry, node));
>> +    while ((entry = io_alloc_cache_get(cache))) {
>> +        free(entry);
> 
> We don't need brackets here.

The extra brackets are required if we are assignments in if, otherwise
the compiler raises a warning (bugprone-assignment-in-if-condition)

> Personally, I don't have anything
> against assignments in if, but it's probably better to avoid them

Sure. I will remove the assignents in "if" part and maybe replicate what
we have
in io_alloc_cache_get(). Something as:
       if (cache->list.next) {
               node = cache->list.next;

Thanks for the review!

Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[Pavel Begunkov]

On 2/21/23 18:38, Breno Leitao wrote:
> On 21/02/2023 17:45, Pavel Begunkov wrote:
>> On 2/21/23 13:57, Breno Leitao wrote:
>>> Having cache entries linked using the hlist format brings no benefit, and
>>> also requires an unnecessary extra pointer address per cache entry.
>>>
>>> Use the internal io_wq_work_node single-linked list for the internal
>>> alloc caches (async_msghdr and async_poll)
>>>
>>> This is required to be able to use KASAN on cache entries, since we do
>>> not need to touch unused (and poisoned) cache entries when adding more
>>> entries to the list.
>>
>> Looks good, a few nits
>>
>>>
>>> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>> ---
>>>    include/linux/io_uring_types.h |  2 +-
>>>    io_uring/alloc_cache.h         | 27 +++++++++++++++------------
>>>    2 files changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/io_uring_types.h
>>> b/include/linux/io_uring_types.h
>>> index 0efe4d784358..efa66b6c32c9 100644
>>> --- a/include/linux/io_uring_types.h
>>> +++ b/include/linux/io_uring_types.h
>>> @@ -188,7 +188,7 @@ struct io_ev_fd {
>>>    };
>>>    
>> [...]
>>> -    if (!hlist_empty(&cache->list)) {
>>> -        struct hlist_node *node = cache->list.first;
>>> -
>>> -        hlist_del(node);
>>> -        return container_of(node, struct io_cache_entry, node);
>>> +    struct io_wq_work_node *node;
>>> +    struct io_cache_entry *entry;
>>> +
>>> +    if (cache->list.next) {
>>> +        node = cache->list.next;
>>> +        entry = container_of(node, struct io_cache_entry, node);
>>
>> I'd prefer to get rid of the node var, it'd be a bit cleaner
>> than keeping two pointers to the same chunk.
>>
>> entry = container_of(node, struct io_cache_entry,
>>                       cache->list.next);
>>
>>> +        cache->list.next = node->next;
>>> +        return entry;
>>>        }
>>>          return NULL;
>>> @@ -35,19 +38,19 @@ static inline struct io_cache_entry
>>> *io_alloc_cache_get(struct io_alloc_cache *c
>>>      static inline void io_alloc_cache_init(struct io_alloc_cache *cache)
>>>    {
>>> -    INIT_HLIST_HEAD(&cache->list);
>>> +    cache->list.next = NULL;
>>>        cache->nr_cached = 0;
>>>    }
>>>      static inline void io_alloc_cache_free(struct io_alloc_cache *cache,
>>>                        void (*free)(struct io_cache_entry *))
>>>    {
>>> -    while (!hlist_empty(&cache->list)) {
>>> -        struct hlist_node *node = cache->list.first;
>>> +    struct io_cache_entry *entry;
>>>    -        hlist_del(node);
>>> -        free(container_of(node, struct io_cache_entry, node));
>>> +    while ((entry = io_alloc_cache_get(cache))) {
>>> +        free(entry);
>>
>> We don't need brackets here.
> 
> The extra brackets are required if we are assignments in if, otherwise
> the compiler raises a warning (bugprone-assignment-in-if-condition)

I mean braces / curly brackets.
>> Personally, I don't have anything
>> against assignments in if, but it's probably better to avoid them
> 
> Sure. I will remove the assignents in "if" part and maybe replicate what
> we have
> in io_alloc_cache_get(). Something as:
>         if (cache->list.next) {
>                 node = cache->list.next;
> 
> Thanks for the review!

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: Move from hlist to io_wq_work_node

[Jens Axboe]

On 2/21/23 11:38?AM, Breno Leitao wrote:
> Sure. I will remove the assignents in "if" part and maybe replicate what
> we have
> in io_alloc_cache_get(). Something as:
>        if (cache->list.next) {
>                node = cache->list.next;
> 
> Thanks for the review!

Pavel is right in that we generally discourage assignments in if
statements etc. For the above, the usual approach would be:

node = cache->list.next;
if (node) {
	...
}

-- 
Jens Axboe