mbox series

[v3,0/4] floppy: suppress UBSAN warning in setup_rw_floppy()

Message ID 20200501134416.72248-1-efremov@linux.com (mailing list archive)
Headers show
Series floppy: suppress UBSAN warning in setup_rw_floppy() | expand

Message

Denis Efremov (Oracle) May 1, 2020, 1:44 p.m. UTC 
These patches are based on Willy's cleanup patches
https://lkml.org/lkml/2020/3/31/609.

The first patch removes pr_cont() in setup_DMA() and prints the contents of
cmd buffer with print_hex_dump(). The last patch also touches these lines
and changes cmd buffer to fullcmd. The 2,3 patches introduce defines to
make it more clear why cmd_count in struct floppy_raw_cmd allows
out-of-bounds access for cmd, reply_count, reply fields. Last patch
handles the warning.

There is the checkpatch warning because of the line length 81 > 80 in
the definition of valid_floppy_drive_params(). This function
intentionally uses autodetect[8] as argument to highlight that this
is exactly the autodetect field from structs floppy_drive_params and
compat_floppy_drive_params. I think that this warning is not a big
problem since next function definition is 85 chars long and whole
driver requires "restyling".

Changes in v3:
 - fix indentation in the second patch
 - remove FD_RAW_REPLY_SIZE from print_hex_dump() in the third patch

Changes in v2:
 - add FD_RAW_CMD_FULLSIZE to the second patch
 - add union with fullcmd to struct floppy_raw_cmd in the last patch

Denis Efremov (4):
  floppy: use print_hex_dump() in setup_DMA()
  floppy: add FD_AUTODETECT_SIZE define for struct floppy_drive_params
  floppy: add defines for sizes of cmd & reply buffers of floppy_raw_cmd
  floppy: suppress UBSAN warning in setup_rw_floppy()

 drivers/block/floppy.c  | 39 ++++++++++++++-------------------------
 include/uapi/linux/fd.h | 26 ++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 29 deletions(-)

Comments

Joe Perches May 1, 2020, 6:02 p.m. UTC | #1 
On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
> These patches are based on Willy's cleanup patches
> https://lkml.org/lkml/2020/3/31/609.

Maybe add pr_fmt and clean up a few messages so
all the logging output is prefixed too.

---
 drivers/block/floppy.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c3daa64..b26bb1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -145,6 +145,8 @@
  * Better audit of register_blkdev.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #undef  FLOPPY_SILENT_DCL_CLEAR
 
 #define REALLY_SLOW_IO
@@ -1724,7 +1726,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id)
 	if (current_fdc >= N_FDC || fdc_state[current_fdc].address == -1) {
 		/* we don't even know which FDC is the culprit */
 		pr_info("DOR0=%x\n", fdc_state[0].dor);
-		pr_info("floppy interrupt on bizarre fdc %d\n", current_fdc);
+		pr_info("interrupt on bizarre fdc %d\n", current_fdc);
 		pr_info("handler=%ps\n", handler);
 		is_alive(__func__, "bizarre fdc");
 		return IRQ_NONE;
@@ -2276,7 +2278,7 @@ static void request_done(int uptodate)
 	reschedule_timeout(MAXTIMEOUT, msg);
 
 	if (!req) {
-		pr_info("floppy.c: no request in request_done\n");
+		pr_info("no request in request_done\n");
 		return;
 	}
 
@@ -4181,8 +4183,7 @@ static void floppy_rb0_cb(struct bio *bio)
 	int drive = cbdata->drive;
 
 	if (bio->bi_status) {
-		pr_info("floppy: error %d while reading block 0\n",
-			bio->bi_status);
+		pr_info("error %d while reading block 0\n", bio->bi_status);
 		set_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
 	}
 	complete(&cbdata->complete);
@@ -4954,7 +4955,7 @@ static void floppy_release_irq_and_dma(void)
 #endif
 
 	if (delayed_work_pending(&fd_timeout))
-		pr_info("floppy timer still active:%s\n", timeout_message);
+		pr_info("timer still active:%s\n", timeout_message);
 	if (delayed_work_pending(&fd_timer))
 		pr_info("auxiliary floppy timer still active\n");
 	if (work_pending(&floppy_work))
Denis Efremov (Oracle) May 1, 2020, 6:22 p.m. UTC | #2 
On 5/1/20 9:02 PM, Joe Perches wrote:
> On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
>> These patches are based on Willy's cleanup patches
>> https://lkml.org/lkml/2020/3/31/609.
> 
> Maybe add pr_fmt and clean up a few messages so
> all the logging output is prefixed too.
>

Yes, I'm preparing next patchset with almost the same pr_fmt patch in it.
However, simply adding pr_fmt is not enough. We need to remove DPRINT macro
from the driver because it uses current_drive and this is wrong after Willy's cleanups.

#define DPRINT(format, args...) \                                                
        pr_info("floppy%d: " format, current_drive, ##args)

I also don't like debug_dcl and how it is used.
 
> ---
>  drivers/block/floppy.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index c3daa64..b26bb1 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -145,6 +145,8 @@
>   * Better audit of register_blkdev.
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #undef  FLOPPY_SILENT_DCL_CLEAR
>  
>  #define REALLY_SLOW_IO
> @@ -1724,7 +1726,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id)
>  	if (current_fdc >= N_FDC || fdc_state[current_fdc].address == -1) {
>  		/* we don't even know which FDC is the culprit */
>  		pr_info("DOR0=%x\n", fdc_state[0].dor);
> -		pr_info("floppy interrupt on bizarre fdc %d\n", current_fdc);
> +		pr_info("interrupt on bizarre fdc %d\n", current_fdc);
>  		pr_info("handler=%ps\n", handler);
>  		is_alive(__func__, "bizarre fdc");
>  		return IRQ_NONE;
> @@ -2276,7 +2278,7 @@ static void request_done(int uptodate)
>  	reschedule_timeout(MAXTIMEOUT, msg);
>  
>  	if (!req) {
> -		pr_info("floppy.c: no request in request_done\n");
> +		pr_info("no request in request_done\n");
>  		return;
>  	}
>  
> @@ -4181,8 +4183,7 @@ static void floppy_rb0_cb(struct bio *bio)
>  	int drive = cbdata->drive;
>  
>  	if (bio->bi_status) {
> -		pr_info("floppy: error %d while reading block 0\n",
> -			bio->bi_status);
> +		pr_info("error %d while reading block 0\n", bio->bi_status);
>  		set_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags);
>  	}
>  	complete(&cbdata->complete);
> @@ -4954,7 +4955,7 @@ static void floppy_release_irq_and_dma(void)
>  #endif
>  
>  	if (delayed_work_pending(&fd_timeout))
> -		pr_info("floppy timer still active:%s\n", timeout_message);
> +		pr_info("timer still active:%s\n", timeout_message);
>  	if (delayed_work_pending(&fd_timer))
>  		pr_info("auxiliary floppy timer still active\n");
>  	if (work_pending(&floppy_work))
> 
>
Joe Perches May 1, 2020, 6:31 p.m. UTC | #3 
On Fri, 2020-05-01 at 21:22 +0300, Denis Efremov wrote:
> On 5/1/20 9:02 PM, Joe Perches wrote:
> > On Fri, 2020-05-01 at 16:44 +0300, Denis Efremov wrote:
> > > These patches are based on Willy's cleanup patches
> > > https://lkml.org/lkml/2020/3/31/609.
> > 
> > Maybe add pr_fmt and clean up a few messages so
> > all the logging output is prefixed too.
> > 
> 
> Yes, I'm preparing next patchset with almost the same pr_fmt patch in it.
> However, simply adding pr_fmt is not enough. We need to remove DPRINT macro
> from the driver because it uses current_drive and this is wrong after Willy's cleanups.
> 
> #define DPRINT(format, args...) \                                                
>         pr_info("floppy%d: " format, current_drive, ##args)
> 
> I also don't like debug_dcl and how it is used.

great, thanks.
Denis Efremov (Oracle) May 6, 2020, 7:33 a.m. UTC | #4 
On 5/1/20 4:44 PM, Denis Efremov wrote
> 
> The first patch removes pr_cont() in setup_DMA() and prints the contents of
> cmd buffer with print_hex_dump(). The last patch also touches these lines
> and changes cmd buffer to fullcmd. The 2,3 patches introduce defines to
> make it more clear why cmd_count in struct floppy_raw_cmd allows
> out-of-bounds access for cmd, reply_count, reply fields. Last patch
> handles the warning.

Applied,

https://github.com/evdenis/linux-floppy/tree/cleanups

Denis