From:   Gabriel Krisman Bertazi <krisman@suse.de>
To:     Breno Leitao <leitao@debian.org>
Cc:     axboe@kernel.dk, asml.silence@gmail.com, io-uring@vger.kernel.org,
        linux-kernel@vger.kernel.org, gustavold@meta.com, leit@meta.com,
        kasan-dev@googlegroups.com
Subject: Re: [PATCH v3 2/2] io_uring: Add KASAN support for alloc_caches
References: <20230223164353.2839177-1-leitao@debian.org>
        <20230223164353.2839177-3-leitao@debian.org>
Date:   Thu, 23 Feb 2023 16:09:24 -0300
In-Reply-To: <20230223164353.2839177-3-leitao@debian.org> (Breno Leitao's
        message of "Thu, 23 Feb 2023 08:43:53 -0800")
Message-ID: <87sfewryfv.fsf@suse.de>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Breno Leitao <leitao@debian.org> writes:

> 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>
> ---
>  include/linux/io_uring_types.h | 1 +
>  io_uring/alloc_cache.h         | 6 +++++-
>  io_uring/io_uring.c            | 4 ++--
>  io_uring/net.h                 | 5 ++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index efa66b6c32c9..35ebcfb46047 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -190,6 +190,7 @@ struct io_ev_fd {
>  struct io_alloc_cache {
>  	struct io_wq_work_node	list;
>  	unsigned int		nr_cached;
> +	size_t			elem_size;
>  };
>  
>  struct io_ring_ctx {
> diff --git a/io_uring/alloc_cache.h b/io_uring/alloc_cache.h
> index 301855e94309..3aba7b356320 100644
> --- a/io_uring/alloc_cache.h
> +++ b/io_uring/alloc_cache.h
> @@ -16,6 +16,8 @@ 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;
> @@ -27,6 +29,7 @@ static inline struct io_cache_entry *io_alloc_cache_get(struct io_alloc_cache *c
>  		struct io_cache_entry *entry;
>  
>  		entry = container_of(cache->list.next, struct io_cache_entry, node);
> +		kasan_unpoison_range(entry, cache->elem_size);

I kind of worry there is no type checking at the same time we are
unpoisoning a constant-size range.  Seems easy to misuse the API.  But it
does look much better now with elem_size cached inside io_alloc_cache.

>  
> -#if defined(CONFIG_NET)
>  struct io_async_msghdr {
> +#if defined(CONFIG_NET)
>  	union {
>  		struct iovec		fast_iov[UIO_FASTIOV];
>  		struct {
> @@ -22,8 +22,11 @@ struct io_async_msghdr {
>  	struct sockaddr __user		*uaddr;
>  	struct msghdr			msg;
>  	struct sockaddr_storage		addr;
> +#endif
>  };
>  
> +#if defined(CONFIG_NET)
> +

Nit, but you could have added an empty definition in the #else section
that already exists in the file, or just guarded the caching code
entirely when CONFIG_NET=n.

Just nits, and overall it is good to have this KASAN support!

Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>

-- 
Gabriel Krisman Bertazi