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 <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
---
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
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 <[email protected]>
---
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> 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
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 <[email protected]>
>> Signed-off-by: Breno Leitao <[email protected]>
>> ---
>> 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!
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 <[email protected]>
>>> Signed-off-by: Breno Leitao <[email protected]>
>>> ---
>>> 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
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