[PATCH 0/5] Block layer patches for kernel v5.8

[PATCH 0/5] Block layer patches for kernel v5.8

[Bart Van Assche]

Hi Jens,

The patches in this series are what I came up with as the result of
analyzing Alexander Potapenko's report about reading from null_blk.
Please consider these patches for kernel v5.8.

Thanks,

Bart.

Bart Van Assche (5):
  block: Fix type of first compat_put_{,u}long() argument
  bio.h: Declare the arguments of bio iteration functions const
  block: Document the bio_vec properties
  block: Fix zero_fill_bio()
  null_blk: Zero-initialize read buffers in non-memory-backed mode

 block/bio.c                   | 27 ++++++++++++++++++++++-----
 block/ioctl.c                 |  4 ++--
 drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
 include/linux/bio.h           |  7 ++++---
 include/linux/bvec.h          | 18 ++++++++++++++++--
 5 files changed, 74 insertions(+), 12 deletions(-)

[PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument

[Bart Van Assche]

This patch fixes the following sparse warnings:

block/ioctl.c:209:16: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:209:16:    expected void const volatile [noderef] <asn:1> *
block/ioctl.c:209:16:    got signed int [usertype] *argp
block/ioctl.c:214:16: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:214:16:    expected void const volatile [noderef] <asn:1> *
block/ioctl.c:214:16:    got unsigned int [usertype] *argp
block/ioctl.c:666:40: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:666:40:    expected signed int [usertype] *argp
block/ioctl.c:666:40:    got void [noderef] <asn:1> *argp
block/ioctl.c:672:41: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:672:41:    expected unsigned int [usertype] *argp
block/ioctl.c:672:41:    got void [noderef] <asn:1> *argp

Cc: Arnd Bergmann <arnd@arndb.de>
Fixes: 9b81648cb5e3 ("compat_ioctl: simplify up block/ioctl.c")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/ioctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 75c64811b534..bdb3bbb253d9 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -204,12 +204,12 @@ static int put_u64(u64 __user *argp, u64 val)
 }
 
 #ifdef CONFIG_COMPAT
-static int compat_put_long(compat_long_t *argp, long val)
+static int compat_put_long(compat_long_t __user *argp, long val)
 {
 	return put_user(val, argp);
 }
 
-static int compat_put_ulong(compat_ulong_t *argp, compat_ulong_t val)
+static int compat_put_ulong(compat_ulong_t __user *argp, compat_ulong_t val)
 {
 	return put_user(val, argp);
 }

[PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const

[Bart Van Assche]

This change makes it possible to pass 'const struct bio *' arguments to
these functions.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: Alexander Potapenko <glider@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/bio.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index a0ee494a6329..58e6134b1c05 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -70,7 +70,7 @@ static inline bool bio_has_data(struct bio *bio)
 	return false;
 }
 
-static inline bool bio_no_advance_iter(struct bio *bio)
+static inline bool bio_no_advance_iter(const struct bio *bio)
 {
 	return bio_op(bio) == REQ_OP_DISCARD ||
 	       bio_op(bio) == REQ_OP_SECURE_ERASE ||
@@ -138,8 +138,8 @@ static inline bool bio_next_segment(const struct bio *bio,
 #define bio_for_each_segment_all(bvl, bio, iter) \
 	for (bvl = bvec_init_iter_all(&iter); bio_next_segment((bio), &iter); )
 
-static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
-				    unsigned bytes)
+static inline void bio_advance_iter(const struct bio *bio,
+				    struct bvec_iter *iter, unsigned bytes)
 {
 	iter->bi_sector += bytes >> 9;
 

[PATCH 3/5] block: Document the bio_vec properties

[Bart Van Assche]

Since it is nontrivial that nth_page() does not have to be used for a
bio_vec, document this.

CC: Christoph Hellwig <hch@infradead.org>
Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/linux/bvec.h | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index a81c13ac1972..25295c11b164 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -12,8 +12,22 @@
 #include <linux/errno.h>
 #include <linux/mm.h>
 
-/*
- * was unsigned short, but we might as well be ready for > 64kB I/O pages
+/**
+ * struct bio_vec - a contiguous range of physical memory addresses
+ * @bv_page:   First page associated with the address range.
+ * @bv_len:    Number of bytes in the address range.
+ * @bv_offset: Start of the address range relative to the start of @bv_page.
+ *
+ * The following holds for a bvec if n * PAGE_SIZE < bv_offset + bv_len:
+ *
+ *   nth_page(@bv_page, n) == @bv_page + n
+ *
+ * From the description of commit 52d52d1c98a9 ("block: only allow contiguous
+ * page structs in a bio_vec"): "We currently have to call nth_page when
+ * iterating over pages inside a bio_vec.  Jens complained a while ago that
+ * this is fairly expensive. To mitigate this we can check that that the
+ * actual page structures are contiguous when adding them to the bio, and just
+ * do check pointer arithmetics later on." See also page_is_mergeable().
  */
 struct bio_vec {
 	struct page	*bv_page;

[PATCH 4/5] block: Fix zero_fill_bio()

[Bart Van Assche]

Multiple block drivers use zero_fill_bio() to zero-initialize the data
buffer used for read operations. Make sure that all pages are zeroed
instead of only the first if one or more multi-page bvecs are used to
describe the data buffer.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/bio.c         | 27 ++++++++++++++++++++++-----
 include/linux/bio.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 1594804fe8bc..48fcafbdae70 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -527,17 +527,34 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
 }
 EXPORT_SYMBOL(bio_alloc_bioset);
 
+void zero_fill_bvec(const struct bio_vec *bvec)
+{
+	struct page *page = bvec->bv_page;
+	u32 offset = bvec->bv_offset;
+	u32 left = bvec->bv_len;
+
+	while (left) {
+		u32 len = min_t(u32, left, PAGE_SIZE - offset);
+		void *kaddr;
+
+		kaddr = kmap_atomic(page);
+		memset(kaddr + offset, 0, len);
+		flush_dcache_page(page);
+		kunmap_atomic(kaddr);
+		page++;
+		left -= len;
+		offset = 0;
+	}
+}
+EXPORT_SYMBOL(zero_fill_bvec);
+
 void zero_fill_bio_iter(struct bio *bio, struct bvec_iter start)
 {
-	unsigned long flags;
 	struct bio_vec bv;
 	struct bvec_iter iter;
 
 	__bio_for_each_segment(bv, bio, iter, start) {
-		char *data = bvec_kmap_irq(&bv, &flags);
-		memset(data, 0, bv.bv_len);
-		flush_dcache_page(bv.bv_page);
-		bvec_kunmap_irq(data, &flags);
+		zero_fill_bvec(&bv);
 	}
 }
 EXPORT_SYMBOL(zero_fill_bio_iter);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 58e6134b1c05..9438cbdfa19e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -455,6 +455,7 @@ extern void bio_copy_data_iter(struct bio *dst, struct bvec_iter *dst_iter,
 extern void bio_copy_data(struct bio *dst, struct bio *src);
 extern void bio_list_copy_data(struct bio *dst, struct bio *src);
 extern void bio_free_pages(struct bio *bio);
+void zero_fill_bvec(const struct bio_vec *bvec);
 void zero_fill_bio_iter(struct bio *bio, struct bvec_iter iter);
 void bio_truncate(struct bio *bio, unsigned new_size);
 void guard_bio_eod(struct bio *bio);

[PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Bart Van Assche]

This patch suppresses an uninteresting KMSAN complaint without affecting
performance of the null_blk driver if CONFIG_KMSAN is disabled.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: Alexander Potapenko <glider@google.com>
Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 06f5761fccb6..df1e144eeaa4 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
 	return errno_to_blk_status(err);
 }
 
+static void nullb_zero_rq_data_buffer(const struct request *rq)
+{
+	struct req_iterator iter;
+	struct bio_vec bvec;
+
+	rq_for_each_bvec(bvec, rq, iter)
+		zero_fill_bvec(&bvec);
+}
+
+static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
+{
+	struct nullb_device *dev = cmd->nq->dev;
+
+	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
+		zero_fill_bio(cmd->bio);
+	else if (req_op(cmd->rq) == REQ_OP_READ)
+		nullb_zero_rq_data_buffer(cmd->rq);
+}
+
+/* Complete a request. Only called if dev->memory_backed == 0. */
 static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
 {
+	/*
+	 * Since root privileges are required to configure the null_blk
+	 * driver, it is fine that this driver does not initialize the
+	 * data buffers of read commands. Zero-initialize these buffers
+	 * anyway if KMSAN is enabled to prevent that KMSAN complains
+	 * about null_blk not initializing read data buffers.
+	 */
+	if (IS_ENABLED(CONFIG_KMSAN))
+		nullb_zero_read_cmd_buffer(cmd);
+
 	/* Complete IO by inline, softirq or timer */
 	switch (cmd->nq->dev->irqmode) {
 	case NULL_IRQ_SOFTIRQ:

Re: [PATCH 4/5] block: Fix zero_fill_bio()

[Ming Lei]

On Fri, May 15, 2020 at 05:19:13PM -0700, Bart Van Assche wrote:
> Multiple block drivers use zero_fill_bio() to zero-initialize the data
> buffer used for read operations. Make sure that all pages are zeroed
> instead of only the first if one or more multi-page bvecs are used to
> describe the data buffer.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/bio.c         | 27 ++++++++++++++++++++++-----
>  include/linux/bio.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 1594804fe8bc..48fcafbdae70 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -527,17 +527,34 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
>  }
>  EXPORT_SYMBOL(bio_alloc_bioset);
>  
> +void zero_fill_bvec(const struct bio_vec *bvec)
> +{
> +	struct page *page = bvec->bv_page;
> +	u32 offset = bvec->bv_offset;
> +	u32 left = bvec->bv_len;
> +
> +	while (left) {
> +		u32 len = min_t(u32, left, PAGE_SIZE - offset);
> +		void *kaddr;
> +
> +		kaddr = kmap_atomic(page);
> +		memset(kaddr + offset, 0, len);
> +		flush_dcache_page(page);
> +		kunmap_atomic(kaddr);
> +		page++;
> +		left -= len;
> +		offset = 0;
> +	}
> +}
> +EXPORT_SYMBOL(zero_fill_bvec);
> +
>  void zero_fill_bio_iter(struct bio *bio, struct bvec_iter start)
>  {
> -	unsigned long flags;
>  	struct bio_vec bv;
>  	struct bvec_iter iter;
>  
>  	__bio_for_each_segment(bv, bio, iter, start) {

The 'bv' from __bio_for_each_segment() is single page bvec, and so far
only [__]bio_for_each_bvec iterates over multi-page bvec.

So nothing to be fixed and this patch isn't necessary.

Thanks, 
Ming

Re: [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument

[Arnd Bergmann]

On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch fixes the following sparse warnings:
>
> block/ioctl.c:209:16: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:209:16:    expected void const volatile [noderef] <asn:1> *
> block/ioctl.c:209:16:    got signed int [usertype] *argp
> block/ioctl.c:214:16: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:214:16:    expected void const volatile [noderef] <asn:1> *
> block/ioctl.c:214:16:    got unsigned int [usertype] *argp
> block/ioctl.c:666:40: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:666:40:    expected signed int [usertype] *argp
> block/ioctl.c:666:40:    got void [noderef] <asn:1> *argp
> block/ioctl.c:672:41: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:672:41:    expected unsigned int [usertype] *argp
> block/ioctl.c:672:41:    got void [noderef] <asn:1> *argp
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Fixes: 9b81648cb5e3 ("compat_ioctl: simplify up block/ioctl.c")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Thanks for the fix!

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const

[Alexander Potapenko]

On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/linux/bio.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index a0ee494a6329..58e6134b1c05 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -70,7 +70,7 @@ static inline bool bio_has_data(struct bio *bio)
>         return false;
>  }
>
> -static inline bool bio_no_advance_iter(struct bio *bio)
> +static inline bool bio_no_advance_iter(const struct bio *bio)
>  {
>         return bio_op(bio) == REQ_OP_DISCARD ||
>                bio_op(bio) == REQ_OP_SECURE_ERASE ||
> @@ -138,8 +138,8 @@ static inline bool bio_next_segment(const struct bio *bio,
>  #define bio_for_each_segment_all(bvl, bio, iter) \
>         for (bvl = bvec_init_iter_all(&iter); bio_next_segment((bio), &iter); )
>
> -static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> -                                   unsigned bytes)
> +static inline void bio_advance_iter(const struct bio *bio,
> +                                   struct bvec_iter *iter, unsigned bytes)
>  {
>         iter->bi_sector += bytes >> 9;
On a related note, should this 9 be SECTOR_SHIFT?


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Alexander Potapenko]

On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch suppresses an uninteresting KMSAN complaint without affecting
> performance of the null_blk driver if CONFIG_KMSAN is disabled.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Alexander Potapenko <glider@google.com>

> ---
>  drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 06f5761fccb6..df1e144eeaa4 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
>         return errno_to_blk_status(err);
>  }
>
> +static void nullb_zero_rq_data_buffer(const struct request *rq)
> +{
> +       struct req_iterator iter;
> +       struct bio_vec bvec;
> +
> +       rq_for_each_bvec(bvec, rq, iter)
> +               zero_fill_bvec(&bvec);
> +}
> +
> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
> +{
> +       struct nullb_device *dev = cmd->nq->dev;
> +
> +       if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
> +               zero_fill_bio(cmd->bio);
> +       else if (req_op(cmd->rq) == REQ_OP_READ)
> +               nullb_zero_rq_data_buffer(cmd->rq);
> +}
> +
> +/* Complete a request. Only called if dev->memory_backed == 0. */
>  static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
>  {
> +       /*
> +        * Since root privileges are required to configure the null_blk
> +        * driver, it is fine that this driver does not initialize the
> +        * data buffers of read commands. Zero-initialize these buffers
> +        * anyway if KMSAN is enabled to prevent that KMSAN complains
> +        * about null_blk not initializing read data buffers.
> +        */
> +       if (IS_ENABLED(CONFIG_KMSAN))
> +               nullb_zero_read_cmd_buffer(cmd);
> +
>         /* Complete IO by inline, softirq or timer */
>         switch (cmd->nq->dev->irqmode) {
>         case NULL_IRQ_SOFTIRQ:



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument

[Christoph Hellwig]

On Fri, May 15, 2020 at 05:19:10PM -0700, Bart Van Assche wrote:
> This patch fixes the following sparse warnings:

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const

[Christoph Hellwig]

On Fri, May 15, 2020 at 05:19:11PM -0700, Bart Van Assche wrote:
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/5] block: Document the bio_vec properties

[Christoph Hellwig]

On Fri, May 15, 2020 at 05:19:12PM -0700, Bart Van Assche wrote:
> Since it is nontrivial that nth_page() does not have to be used for a
> bio_vec, document this.

No need to quote the commit in the source code, just add the expanation
and not the source in the commit log for this patch.

Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const

[Bart Van Assche]

On 2020-05-16 01:55, Alexander Potapenko wrote:
> On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>> -static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>> -                                   unsigned bytes)
>> +static inline void bio_advance_iter(const struct bio *bio,
>> +                                   struct bvec_iter *iter, unsigned bytes)
>>  {
>>         iter->bi_sector += bytes >> 9;
>
> On a related note, should this 9 be SECTOR_SHIFT?

Hi Alexander,

I think a patch series is already under review for replacing "9" with
SECTOR_SHIFT. See also
https://lore.kernel.org/linux-block/20200507075100.1779-1-thunder.leizhen@huawei.com/.

Thanks,

Bart.

Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const

[Chaitanya Kulkarni]

On 5/15/20 5:19 PM, Bart Van Assche wrote:
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.
> 
> Cc: Christoph Hellwig<hch@lst.de>
> Cc: Ming Lei<ming.lei@redhat.com>
> Cc: Damien Le Moal<damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko<glider@google.com>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>

Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Damien Le Moal]

On 2020/05/16 9:19, Bart Van Assche wrote:
> This patch suppresses an uninteresting KMSAN complaint without affecting
> performance of the null_blk driver if CONFIG_KMSAN is disabled.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 06f5761fccb6..df1e144eeaa4 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
>  	return errno_to_blk_status(err);
>  }
>  
> +static void nullb_zero_rq_data_buffer(const struct request *rq)
> +{
> +	struct req_iterator iter;
> +	struct bio_vec bvec;
> +
> +	rq_for_each_bvec(bvec, rq, iter)
> +		zero_fill_bvec(&bvec);
> +}
> +
> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
> +{
> +	struct nullb_device *dev = cmd->nq->dev;
> +
> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
> +		zero_fill_bio(cmd->bio);
> +	else if (req_op(cmd->rq) == REQ_OP_READ)
> +		nullb_zero_rq_data_buffer(cmd->rq);
> +}

Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?

> +
> +/* Complete a request. Only called if dev->memory_backed == 0. */
>  static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
>  {
> +	/*
> +	 * Since root privileges are required to configure the null_blk
> +	 * driver, it is fine that this driver does not initialize the
> +	 * data buffers of read commands. Zero-initialize these buffers
> +	 * anyway if KMSAN is enabled to prevent that KMSAN complains
> +	 * about null_blk not initializing read data buffers.
> +	 */
> +	if (IS_ENABLED(CONFIG_KMSAN))
> +		nullb_zero_read_cmd_buffer(cmd);
> +
>  	/* Complete IO by inline, softirq or timer */
>  	switch (cmd->nq->dev->irqmode) {
>  	case NULL_IRQ_SOFTIRQ:
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Bart Van Assche]

On 2020-05-17 18:12, Damien Le Moal wrote:
> On 2020/05/16 9:19, Bart Van Assche wrote:
>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>> +{
>> +	struct req_iterator iter;
>> +	struct bio_vec bvec;
>> +
>> +	rq_for_each_bvec(bvec, rq, iter)
>> +		zero_fill_bvec(&bvec);
>> +}
>> +
>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>> +{
>> +	struct nullb_device *dev = cmd->nq->dev;
>> +
>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>> +		zero_fill_bio(cmd->bio);
>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>> +		nullb_zero_rq_data_buffer(cmd->rq);
>> +}
> 
> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?

Hi Damien,

It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
#ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
expose the above code to the build robot.

Thanks,

Bart.

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Damien Le Moal]

On 2020/05/18 10:32, Bart Van Assche wrote:
> On 2020-05-17 18:12, Damien Le Moal wrote:
>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>> +{
>>> +	struct req_iterator iter;
>>> +	struct bio_vec bvec;
>>> +
>>> +	rq_for_each_bvec(bvec, rq, iter)
>>> +		zero_fill_bvec(&bvec);
>>> +}
>>> +
>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>> +{
>>> +	struct nullb_device *dev = cmd->nq->dev;
>>> +
>>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>> +		zero_fill_bio(cmd->bio);
>>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>>> +		nullb_zero_rq_data_buffer(cmd->rq);
>>> +}
>>
>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
> 
> Hi Damien,
> 
> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
> expose the above code to the build robot.

But then you will get a "defined but unused" build warning, no ?

> 
> Thanks,
> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Bart Van Assche]

On 2020-05-17 19:10, Damien Le Moal wrote:
> On 2020/05/18 10:32, Bart Van Assche wrote:
>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>> +{
>>>> +	struct req_iterator iter;
>>>> +	struct bio_vec bvec;
>>>> +
>>>> +	rq_for_each_bvec(bvec, rq, iter)
>>>> +		zero_fill_bvec(&bvec);
>>>> +}
>>>> +
>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>> +{
>>>> +	struct nullb_device *dev = cmd->nq->dev;
>>>> +
>>>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>> +		zero_fill_bio(cmd->bio);
>>>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>>>> +		nullb_zero_rq_data_buffer(cmd->rq);
>>>> +}
>>>
>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>
>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>> expose the above code to the build robot.
> 
> But then you will get a "defined but unused" build warning, no ?

Not when using IS_ENABLED(...).

Bart.

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Damien Le Moal]

On 2020/05/18 11:56, Bart Van Assche wrote:
> On 2020-05-17 19:10, Damien Le Moal wrote:
>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>> +{
>>>>> +	struct req_iterator iter;
>>>>> +	struct bio_vec bvec;
>>>>> +
>>>>> +	rq_for_each_bvec(bvec, rq, iter)
>>>>> +		zero_fill_bvec(&bvec);
>>>>> +}
>>>>> +
>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>> +{
>>>>> +	struct nullb_device *dev = cmd->nq->dev;
>>>>> +
>>>>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>> +		zero_fill_bio(cmd->bio);
>>>>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>> +		nullb_zero_rq_data_buffer(cmd->rq);
>>>>> +}
>>>>
>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>
>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>> expose the above code to the build robot.
>>
>> But then you will get a "defined but unused" build warning, no ?
> 
> Not when using IS_ENABLED(...).

I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
CONFIG_KMSAN is not enabled/defined, but the function definitions will still
remain, won't they ? That will lead to "defined but unused" warning, no ? What
am I missing here ?

> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Bart Van Assche]

On 2020-05-17 20:12, Damien Le Moal wrote:
> On 2020/05/18 11:56, Bart Van Assche wrote:
>> On 2020-05-17 19:10, Damien Le Moal wrote:
>>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>>> +{
>>>>>> +	struct req_iterator iter;
>>>>>> +	struct bio_vec bvec;
>>>>>> +
>>>>>> +	rq_for_each_bvec(bvec, rq, iter)
>>>>>> +		zero_fill_bvec(&bvec);
>>>>>> +}
>>>>>> +
>>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>>> +{
>>>>>> +	struct nullb_device *dev = cmd->nq->dev;
>>>>>> +
>>>>>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>>> +		zero_fill_bio(cmd->bio);
>>>>>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>>> +		nullb_zero_rq_data_buffer(cmd->rq);
>>>>>> +}
>>>>>
>>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>>
>>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>>> expose the above code to the build robot.
>>>
>>> But then you will get a "defined but unused" build warning, no ?
>>
>> Not when using IS_ENABLED(...).
> 
> I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
> CONFIG_KMSAN is not enabled/defined, but the function definitions will still
> remain, won't they ? That will lead to "defined but unused" warning, no ? What
> am I missing here ?

"if (IS_ENABLED(CONFIG_KMSAN))" won't be removed by the preprocessor.
The preprocessor will convert it into if (0).

This is what I found in the gcc documentation about -Wunused-function:
"-Wunused-function
Warn whenever a static function is declared but not defined or a
non-inline static function is unused. This warning is enabled by -Wall."
I think that "if (0) func(...)" counts as using func().

Bart.

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Damien Le Moal]

On 2020/05/18 23:31, Bart Van Assche wrote:
> On 2020-05-17 20:12, Damien Le Moal wrote:
>> On 2020/05/18 11:56, Bart Van Assche wrote:
>>> On 2020-05-17 19:10, Damien Le Moal wrote:
>>>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>>>> +{
>>>>>>> +	struct req_iterator iter;
>>>>>>> +	struct bio_vec bvec;
>>>>>>> +
>>>>>>> +	rq_for_each_bvec(bvec, rq, iter)
>>>>>>> +		zero_fill_bvec(&bvec);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>>>> +{
>>>>>>> +	struct nullb_device *dev = cmd->nq->dev;
>>>>>>> +
>>>>>>> +	if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>>>> +		zero_fill_bio(cmd->bio);
>>>>>>> +	else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>>>> +		nullb_zero_rq_data_buffer(cmd->rq);
>>>>>>> +}
>>>>>>
>>>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>>>
>>>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>>>> expose the above code to the build robot.
>>>>
>>>> But then you will get a "defined but unused" build warning, no ?
>>>
>>> Not when using IS_ENABLED(...).
>>
>> I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
>> CONFIG_KMSAN is not enabled/defined, but the function definitions will still
>> remain, won't they ? That will lead to "defined but unused" warning, no ? What
>> am I missing here ?
> 
> "if (IS_ENABLED(CONFIG_KMSAN))" won't be removed by the preprocessor.
> The preprocessor will convert it into if (0).
> 
> This is what I found in the gcc documentation about -Wunused-function:
> "-Wunused-function
> Warn whenever a static function is declared but not defined or a
> non-inline static function is unused. This warning is enabled by -Wall."
> I think that "if (0) func(...)" counts as using func().

Makes sense. Thanks for the explanation.
But from code-size perspective, I think it would still make sense to add the
#ifdef CONFIG_KMSAN around the zeroing functions.


> 
> Bart.
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode

[Bart Van Assche]

On 2020-05-18 20:03, Damien Le Moal wrote:
> Makes sense. Thanks for the explanation.
> But from code-size perspective, I think it would still make sense to add the
> #ifdef CONFIG_KMSAN around the zeroing functions.

Does anyone who cares about kernel size include the null_blk driver? I
would be surprised if anyone who cares about kernel size (e.g. embedded
systems developers) would use anything else than CONFIG_BLK_DEV_NULL_BLK=n.

Thanks,

Bart.