Message-ID: <19b0614ca46.65ed8ae3522798.9016216162556753318@linux.beauty>
Date: Wed, 10 Dec 2025 10:26:24 +0800
From: Li Chen <me@...ux.beauty>
To: "Dongsheng Yang" <dongsheng.yang@...ux.dev>
Cc: "dm-devel" <dm-devel@...ts.linux.dev>,
	"linux-kernel" <linux-kernel@...r.kernel.org>,
	"Zheng Gu" <cengku@...il.com>,
	"Mikulas Patocka" <mpatocka@...hat.com>
Subject: Re: [PATCH] dm pcache: reject mappings larger than backing

Hi Dongsheng,

Thanks for your quick reply.
 ---- On Mon, 08 Dec 2025 19:44:40 +0800  Dongsheng Yang <dongsheng.yang@...ux.dev> wrote --- 
 > 
 > 在 12/8/2025 7:25 PM, Li Chen 写道:
 > > Reject pcache targets whose logical size exceeds the backing device.
 > > Prevent oversized tables from issuing IO past the end of the backing,
 > > which may corrupt memory and cause kernel crash.
 > >
 > > Signed-off-by: Li Chen <me@...ux.beauty>
 > > ---
 > >   drivers/md/dm-pcache/dm_pcache.c | 15 +++++++++++++++
 > >   1 file changed, 15 insertions(+)
 > >
 > > diff --git a/drivers/md/dm-pcache/dm_pcache.c b/drivers/md/dm-pcache/dm_pcache.c
 > > index e5f5936fa6f0..f72d1ba4b740 100644
 > > --- a/drivers/md/dm-pcache/dm_pcache.c
 > > +++ b/drivers/md/dm-pcache/dm_pcache.c
 > > @@ -199,6 +199,8 @@ static int parse_cache_opts(struct dm_pcache *pcache, struct dm_arg_set *as,
 > >   static int pcache_start(struct dm_pcache *pcache, char **error)
 > >   {
 > >       int ret;
 > > +    struct dm_target *ti = pcache->ti;
 > > +    struct pcache_backing_dev *backing_dev;
 > >   
 > >       ret = cache_dev_start(pcache);
 > >       if (ret) {
 > > @@ -212,6 +214,19 @@ static int pcache_start(struct dm_pcache *pcache, char **error)
 > >           goto stop_cache;
 > >       }
 > >   
 > > +    /* Sanity-check: logical size must not exceed backing device size */
 > > +    backing_dev = &pcache->backing_dev;
 > > +    if (ti->len > backing_dev->dev_size) {
 > > +        pcache_dev_err(
 > > +            pcache,
 > > +            "backing device too small: logical=%llu sectors, backing=%llu sectors",
 > > +            (unsigned long long)ti->len,
 > > +            (unsigned long long)backing_dev->dev_size);
 > > +        *error = "Requested mapping exceeds backing device size";
 > > +        ret = -EINVAL;
 > > +        goto stop_backing;
 > > +    }
 > > +
 > 
 > 
 > Thanx for your patch, When developing dm-pcache, I considered whether to 
 > add a check there (for target size vs backing device size). But after 
 > looking at other existing targets, it seems none of them implement such 
 > a check, so I wasn't sure there is a requirement; hence I didn't add it 
 > at that time.

 I did a quick search and found some examples:
https://github.com/torvalds/linux/blob/c9b47175e9131118e6f221cc8fb81397d62e7c91/drivers/md/dm-cache-target.c#L2117
https://github.com/torvalds/linux/blob/c9b47175e9131118e6f221cc8fb81397d62e7c91/drivers/md/dm-verity-target.c#L1416
https://github.com/torvalds/linux/blob/c9b47175e9131118e6f221cc8fb81397d62e7c91/drivers/md/dm-integrity.c#L5166

 > On the other hand, given that the “target size” parameter in the mapping 
 > table is a generic parameter, if we want such a check, it would be 
 > better to implement a generic mechanism in the Device-Mapper core 
 > construction path — provide a hook that allows every target to return a 
 > “maximum creatable target size”, and then in the core path validate the 
 > user-provided target size against that max size before creation.
 > 
 > That is just my personal idea; I hope Mikulas can provide more 
 > information about it.

I just found a common helper called device_area_is_invalid check, already used within target_type->iterate_devices, validates target size. 
Therefore, targets implementing iterate_devices can perform size validation. I confirmed this with a quick test:

modified   drivers/md/dm-pcache/dm_pcache.c
@@ -446,6 +446,15 @@ static int dm_pcache_message(struct dm_target *ti, unsigned int argc,
 	return -EINVAL;
 }
 
+static int pcache_iterate_devices(struct dm_target *ti,
+				  iterate_devices_callout_fn fn, void *data)
+{
+	struct dm_pcache *pcache = ti->private;
+	struct pcache_backing_dev *backing_dev = &pcache->backing_dev;
+
+	return fn(ti, backing_dev->dm_dev, 0, ti->len, data);
+}
+
 static struct target_type dm_pcache_target = {
 	.name		= "pcache",
 	.version	= {0, 1, 0},
@@ -456,6 +465,7 @@ static struct target_type dm_pcache_target = {
 	.map		= dm_pcache_map_bio,
 	.status		= dm_pcache_status,
 	.message	= dm_pcache_message,
+	.iterate_devices = pcache_iterate_devices,
 };
 
 static int __init dm_pcache_init(void)

And it is rejected as expected too:

```
root@...alhost ~# dmsetup create pcache_vdb --table \
                                                                                              "0 31457284 pcache /dev/pmem0 /dev/vdb 4 cache_mode writeback data_crc false"
device-mapper: reload ioctl on pcache_vdb (253:0) failed: Invalid argument
Complete cookie 0xd4d9a60 success.
Complete cookie 0xd4d9a60 success.
Command failed.
root@...alhost ~ [1]# dmesg | tail
[    1.937147] EXT4-fs (vda2): re-mounted dc39223f-5278-4b19-80bc-de1edf4a5a96 r/w.
[    1.941399] systemd-journald[166]: Received client request to flush runtime journal.
[    2.051496] virtio_net virtio0 enp0s3: renamed from eth0
[    2.253929] FAT-fs (vda1): Volume was not properly unmounted. Some data may be corrupt. Please run fsck.
[    2.395796] tsc: Refined TSC clocksource calibration: 3792.905 MHz
[    2.395834] clocksource: tsc: mask: 0xffffffffffffffff max_cycles: 0x6d585308801, max_idle_ns: 881590710907 ns
[    2.396195] clocksource: Switched to clocksource tsc
[  175.269911] device-mapper: table: 253:0: vdb too small for target: start=0, len=31457284, dev_size=31457280
[  175.271582] device-mapper: core: Cannot calculate initial queue limits
[  175.272688] device-mapper: ioctl: unable to set up device queue for new table.
```

Which do you prefer: pcache_iterate_devices or checking in parse_cache_opts? Thanks.

Li​

Powered by blists - more mailing lists