[dm-devel] [bug report] dm flakey: clone pages on write bio before corrupting them

[Mikulas Patocka]

Hi

I don't get it what the problem is. This seems like a false positive.



On Wed, 14 Jun 2023, Dan Carpenter wrote:

> Hello Mikulas Patocka,
> 
> The patch 90ed93c305a0: "dm flakey: clone pages on write bio before
> corrupting them" from May 1, 2023, leads to the following Smatch
> static checker warning:
> 
> 	drivers/md/dm.c:1157 clone_endio()
> 	warn: passing freed memory 'ti'
> 
> drivers/md/dm.c
>     1105 static void clone_endio(struct bio *bio)
>     1106 {
>     1107         blk_status_t error = bio->bi_status;
>     1108         struct dm_target_io *tio = clone_to_tio(bio);
>     1109         struct dm_target *ti = tio->ti;
>     1110         dm_endio_fn endio = ti->type->end_io;
>     1111         struct dm_io *io = tio->io;
>     1112         struct mapped_device *md = io->md;
>     1113 
>     1114         if (unlikely(error == BLK_STS_TARGET)) {
>     1115                 if (bio_op(bio) == REQ_OP_DISCARD &&
>     1116                     !bdev_max_discard_sectors(bio->bi_bdev))
>     1117                         disable_discard(md);
>     1118                 else if (bio_op(bio) == REQ_OP_WRITE_ZEROES &&
>     1119                          !bdev_write_zeroes_sectors(bio->bi_bdev))
>     1120                         disable_write_zeroes(md);
>     1121         }
>     1122 
>     1123         if (static_branch_unlikely(&zoned_enabled) &&
>     1124             unlikely(bdev_is_zoned(bio->bi_bdev)))
>     1125                 dm_zone_endio(io, bio);
>     1126 
>     1127         if (endio) {
>     1128                 int r = endio(ti, bio, &error);
>
> clone_endio() from drivers/md/dm-flakey.c frees "ti".  (Yes, I get
> that "flakey" in the filename means it is supposed to crash but
> presumably not in this way).

Here we call "endio", not "clone_endio". "endio" points to "flakey_end_io" 
and it doesn't free the "ti" structure. It possibly corrupts the read data 
and always return DM_ENDIO_DONE.

clone_endio() in dm-flakey.c calls bio_endio(), bio_endio() calls 
bi_endio, bi_endio points to clone_endio() in dm.c and clone_endio() in 
dm.c decrements the reference count and when the count goes to 0, "ti" may 
be freed.

There are two clone_endio functions, one in dm.c and one in dm-flakey.c. 
Perhaps the static analysis tool mixed them up.

>     1129 
>     1130                 switch (r) {
>     1131                 case DM_ENDIO_REQUEUE:
>     1132                         if (static_branch_unlikely(&zoned_enabled)) {
>     1133                                 /*
>     1134                                  * Requeuing writes to a sequential zone of a zoned
>     1135                                  * target will break the sequential write pattern:
>     1136                                  * fail such IO.
>     1137                                  */
>     1138                                 if (WARN_ON_ONCE(dm_is_zone_write(md, bio)))
>     1139                                         error = BLK_STS_IOERR;
>     1140                                 else
>     1141                                         error = BLK_STS_DM_REQUEUE;
>     1142                         } else
>     1143                                 error = BLK_STS_DM_REQUEUE;
>     1144                         fallthrough;
>     1145                 case DM_ENDIO_DONE:
>     1146                         break;
>     1147                 case DM_ENDIO_INCOMPLETE:
>     1148                         /* The target will handle the io */
>     1149                         return;
>     1150                 default:
>     1151                         DMCRIT("unimplemented target endio return value: %d", r);
>     1152                         BUG();
>     1153                 }
>     1154         }
>     1155 
>     1156         if (static_branch_unlikely(&swap_bios_enabled) &&
> --> 1157             unlikely(swap_bios_limit(ti, bio)))
> 
> Use after free.

There is a reference on the "io" structure held at this point, so "ti" 
cannot be freed here. We will drop the reference later with 
dm_io_dec_pending().

>     1158                 up(&md->swap_bios_semaphore);
>     1159 
>     1160         free_tio(bio);
>     1161         dm_io_dec_pending(io, error);

When we drop the last reference with "dm_io_dec_pending", "ti" may be 
freed. But "dm_io_dec_pending" is the last statement in this function, so 
this should be safe.

dm_io_dec_pending calls __dm_io_dec_pending, there it goes to 
dm_io_complete and __dm_io_complete, there it decrements the in-use 
counter with this_cpu_dec - at this point, "ti" may be freed. But the code 
at this point doesn't access the "ti" variable at all.

Mikulas

>     1162 }
> 
> regards,
> dan carpenter
>