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
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
