| Message ID | 20200516001914.17138-6-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Block layer patches for kernel v5.8 | expand |
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:
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: >
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.
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. >
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.
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. >
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.
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. >
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.
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:
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(+)