| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201215205102.GB6918@magnolia>
Date: Tue, 15 Dec 2020 12:51:02 -0800
From: "Darrick J. Wong" <darrick.wong@...cle.com>
To: Shiyang Ruan <ruansy.fnst@...fujitsu.com>
Cc: linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
linux-nvdimm@...ts.01.org, linux-mm@...ck.org,
linux-fsdevel@...r.kernel.org, linux-raid@...r.kernel.org,
dan.j.williams@...el.com, david@...morbit.com, hch@....de,
song@...nel.org, rgoldwyn@...e.de, qi.fuli@...itsu.com,
y-goto@...itsu.com, "Theodore Ts'o" <tytso@....edu>
Subject: Re: [RFC PATCH v3 8/9] md: Implement ->corrupted_range()
On Tue, Dec 15, 2020 at 08:14:13PM +0800, Shiyang Ruan wrote:
> With the support of ->rmap(), it is possible to obtain the superblock on
> a mapped device.
>
> If a pmem device is used as one target of mapped device, we cannot
> obtain its superblock directly. With the help of SYSFS, the mapped
> device can be found on the target devices. So, we iterate the
> bdev->bd_holder_disks to obtain its mapped device.
>
> Signed-off-by: Shiyang Ruan <ruansy.fnst@...fujitsu.com>
> ---
> drivers/md/dm.c | 66 +++++++++++++++++++++++++++++++++++++++++++
> drivers/nvdimm/pmem.c | 9 ++++--
> fs/block_dev.c | 21 ++++++++++++++
> include/linux/genhd.h | 7 +++++
> 4 files changed, 100 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 4e0cbfe3f14d..9da1f9322735 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -507,6 +507,71 @@ static int dm_blk_report_zones(struct gendisk *disk, sector_t sector,
> #define dm_blk_report_zones NULL
> #endif /* CONFIG_BLK_DEV_ZONED */
>
> +struct dm_blk_corrupt {
> + struct block_device *bdev;
> + sector_t offset;
> +};
> +
> +static int dm_blk_corrupt_fn(struct dm_target *ti, struct dm_dev *dev,
> + sector_t start, sector_t len, void *data)
> +{
> + struct dm_blk_corrupt *bc = data;
> +
> + return bc->bdev == (void *)dev->bdev &&
> + (start <= bc->offset && bc->offset < start + len);
> +}
> +
> +static int dm_blk_corrupted_range(struct gendisk *disk,
> + struct block_device *target_bdev,
> + loff_t target_offset, size_t len, void *data)
> +{
> + struct mapped_device *md = disk->private_data;
> + struct block_device *md_bdev = md->bdev;
> + struct dm_table *map;
> + struct dm_target *ti;
> + struct super_block *sb;
> + int srcu_idx, i, rc = 0;
> + bool found = false;
> + sector_t disk_sec, target_sec = to_sector(target_offset);
> +
> + map = dm_get_live_table(md, &srcu_idx);
> + if (!map)
> + return -ENODEV;
> +
> + for (i = 0; i < dm_table_get_num_targets(map); i++) {
> + ti = dm_table_get_target(map, i);
> + if (ti->type->iterate_devices && ti->type->rmap) {
> + struct dm_blk_corrupt bc = {target_bdev, target_sec};
> +
> + found = ti->type->iterate_devices(ti, dm_blk_corrupt_fn, &bc);
> + if (!found)
> + continue;
> + disk_sec = ti->type->rmap(ti, target_sec);
What happens if the dm device has multiple reverse mappings because the
physical storage is being shared at multiple LBAs? (e.g. a
deduplication target)
> + break;
> + }
> + }
> +
> + if (!found) {
> + rc = -ENODEV;
> + goto out;
> + }
> +
> + sb = get_super(md_bdev);
> + if (!sb) {
> + rc = bd_disk_holder_corrupted_range(md_bdev, to_bytes(disk_sec), len, data);
> + goto out;
> + } else if (sb->s_op->corrupted_range) {
> + loff_t off = to_bytes(disk_sec - get_start_sect(md_bdev));
> +
> + rc = sb->s_op->corrupted_range(sb, md_bdev, off, len, data);
This "call bd_disk_holder_corrupted_range or sb->s_op->corrupted_range"
logic appears twice; should it be refactored into a common helper?
Or, should the superblock dispatch part move to
bd_disk_holder_corrupted_range?
> + }
> + drop_super(sb);
> +
> +out:
> + dm_put_live_table(md, srcu_idx);
> + return rc;
> +}
> +
> static int dm_prepare_ioctl(struct mapped_device *md, int *srcu_idx,
> struct block_device **bdev)
> {
> @@ -3084,6 +3149,7 @@ static const struct block_device_operations dm_blk_dops = {
> .getgeo = dm_blk_getgeo,
> .report_zones = dm_blk_report_zones,
> .pr_ops = &dm_pr_ops,
> + .corrupted_range = dm_blk_corrupted_range,
> .owner = THIS_MODULE
> };
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 4688bff19c20..e8cfaf860149 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -267,11 +267,14 @@ static int pmem_corrupted_range(struct gendisk *disk, struct block_device *bdev,
>
> bdev_offset = (disk_sector - get_start_sect(bdev)) << SECTOR_SHIFT;
> sb = get_super(bdev);
> - if (sb && sb->s_op->corrupted_range) {
> + if (!sb) {
> + rc = bd_disk_holder_corrupted_range(bdev, bdev_offset, len, data);
> + goto out;
> + } else if (sb->s_op->corrupted_range)
> rc = sb->s_op->corrupted_range(sb, bdev, bdev_offset, len, data);
> - drop_super(sb);
This is out of scope for this patch(set) but do you think that the scsi
disk driver should intercept media errors from sense data and call
->corrupted_range too? ISTR Ted muttering that one of his employers had
a patchset to do more with sense data than the upstream kernel currently
does...
> - }
> + drop_super(sb);
>
> +out:
> bdput(bdev);
> return rc;
> }
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9e84b1928b94..d3e6bddb8041 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1171,6 +1171,27 @@ struct bd_holder_disk {
> int refcnt;
> };
>
> +int bd_disk_holder_corrupted_range(struct block_device *bdev, loff_t off, size_t len, void *data)
> +{
> + struct bd_holder_disk *holder;
> + struct gendisk *disk;
> + int rc = 0;
> +
> + if (list_empty(&(bdev->bd_holder_disks)))
> + return -ENODEV;
> +
> + list_for_each_entry(holder, &bdev->bd_holder_disks, list) {
> + disk = holder->disk;
> + if (disk->fops->corrupted_range) {
> + rc = disk->fops->corrupted_range(disk, bdev, off, len, data);
> + if (rc != -ENODEV)
> + break;
> + }
> + }
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(bd_disk_holder_corrupted_range);
> +
> static struct bd_holder_disk *bd_find_holder_disk(struct block_device *bdev,
> struct gendisk *disk)
> {
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index ed06209008b8..fba247b852fa 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -382,9 +382,16 @@ int blkdev_ioctl(struct block_device *, fmode_t, unsigned, unsigned long);
> long compat_blkdev_ioctl(struct file *, unsigned, unsigned long);
>
> #ifdef CONFIG_SYSFS
> +int bd_disk_holder_corrupted_range(struct block_device *bdev, loff_t off,
> + size_t len, void *data);
> int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk);
> void bd_unlink_disk_holder(struct block_device *bdev, struct gendisk *disk);
> #else
> +int bd_disk_holder_corrupted_range(struct block_device *bdev, loff_t off,
> + size_t len, void *data)
> +{
> + return 0;
> +}
> static inline int bd_link_disk_holder(struct block_device *bdev,
> struct gendisk *disk)
> {
> --
> 2.29.2
>
>
>
Powered by blists - more mailing lists