| 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: <SN4PR0401MB3598163C941976D90F745D8B9BF40@SN4PR0401MB3598.namprd04.prod.outlook.com>
Date: Tue, 1 Dec 2020 09:57:24 +0000
From: Johannes Thumshirn <Johannes.Thumshirn@....com>
To: SelvaKumar S <selvakuma.s1@...sung.com>,
"linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>
CC: "kbusch@...nel.org" <kbusch@...nel.org>,
"axboe@...nel.dk" <axboe@...nel.dk>,
Damien Le Moal <Damien.LeMoal@....com>,
"hch@....de" <hch@....de>, "sagi@...mberg.me" <sagi@...mberg.me>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"selvajove@...il.com" <selvajove@...il.com>,
"nj.shetty@...sung.com" <nj.shetty@...sung.com>,
"joshi.k@...sung.com" <joshi.k@...sung.com>,
"javier.gonz@...sung.com" <javier.gonz@...sung.com>
Subject: Re: [RFC PATCH 1/2] block: add simple copy support
On 01/12/2020 08:14, SelvaKumar S wrote:
> +static inline int bio_check_copy_eod(struct bio *bio, sector_t start,
> + sector_t nr_sectors, sector_t maxsector)
> +{
> + if (nr_sectors && maxsector && (nr_sectors > maxsector ||
> + start > maxsector - nr_sectors)) {
Nit: I don't like the line break here, maybe:
if (nr_sectors && maxsector &&
(nr_sectors > maxsector || start > maxsector - nr_sectors)) {
> + handle_bad_sector(bio, maxsector);
> + return -EIO;
> + }
> + return 0;
> +}
> +
> /*
> * Check whether this bio extends beyond the end of the device or partition.
> * This may well happen - the kernel calls bread() without checking the size of
> @@ -737,6 +748,75 @@ static inline int bio_check_eod(struct bio *bio, sector_t maxsector)
> return 0;
> }
>
> +/*
> + * Check for copy limits and remap source ranges if needed.
> + */
> +static inline int blk_check_copy(struct bio *bio)
That function is a bit big to be marked as inline, isn't it?
> +{
> + struct hd_struct *p = NULL;
> + struct request_queue *q = bio->bi_disk->queue;
> + struct blk_copy_payload *payload;
> + unsigned short nr_range;
> + int ret = -EIO;
> + int i, copy_len = 0;
> +
> + rcu_read_lock();
> +
> + if (bio->bi_partno) {
> + p = __disk_get_part(bio->bi_disk, bio->bi_partno);
> + if (unlikely(!p))
> + goto out;
> + if (unlikely(bio_check_ro(bio, p)))
> + goto out;
> + } else {
> + if (unlikely(bio_check_ro(bio, &bio->bi_disk->part0)))
> + goto out;
> + }
> +
> + payload = bio_data(bio);
> + nr_range = payload->copy_range;
> +
> + /* cannot handle copy crossing nr_ranges limit */
> + if (payload->copy_range > q->limits.max_copy_nr_ranges)
> + goto out;
> +
> + for (i = 0; i < nr_range; i++) {
> + copy_len += payload->range[i].len;
> + if (p) {
> + if (bio_check_copy_eod(bio, payload->range[i].src,
> + payload->range[i].len, part_nr_sects_read(p)))
> + goto out;
> + payload->range[i].src += p->start_sect;
> + } else {
> + if (unlikely(bio_check_copy_eod(bio, payload->range[i].src,
> + payload->range[i].len,
> + get_capacity(bio->bi_disk))))
> + goto out;
> + }
> + }
> +
> + /* cannot handle copy more than copy limits */
> + if (copy_len > q->limits.max_copy_sectors)
> + goto out;
> +
> + if (p) {
> + if (unlikely(bio_check_copy_eod(bio, bio->bi_iter.bi_sector, copy_len,
> + part_nr_sects_read(p))))
> + goto out;
> + } else {
> + if (unlikely(bio_check_copy_eod(bio, bio->bi_iter.bi_sector, copy_len,
> + get_capacity(bio->bi_disk))))
> + goto out;
> +
> + }
> +
All these if (p) {} else {} branches make this function a bit hard to follow.
> + if (p)
> + bio->bi_partno = 0;
> + ret = 0;
> +out:
> + rcu_read_unlock();
> + return ret;
> +}
> /*
> * Remap block n of partition p to block n+start(p) of the disk.
> */
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index e90614fd8d6a..db4947f7014d 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -150,6 +150,122 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> }
> EXPORT_SYMBOL(blkdev_issue_discard);
>
> +int __blkdev_issue_copy(struct block_device *bdev, sector_t dest,
> + sector_t nr_srcs, struct range_entry *rlist, gfp_t gfp_mask,
> + int flags, struct bio **biop)
> +{
> + struct request_queue *q = bdev_get_queue(bdev);
> + struct bio *bio;
> + struct blk_copy_payload *payload;
> + unsigned int op;
I don't think op is needed.
> + sector_t bs_mask;
> + sector_t src_sects, len = 0, total_len = 0;
> + int i, ret, total_size;
> +
> + if (!q)
> + return -ENXIO;
> +
> + if (!nr_srcs)
> + return -EINVAL;
> +
> + if (bdev_read_only(bdev))
> + return -EPERM;
> +
> + if (!blk_queue_copy(q))
> + return -EOPNOTSUPP;
> + op = REQ_OP_COPY;
> +
> + bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
> + if (dest & bs_mask)
> + return -EINVAL;
> +
> + payload = kmalloc(sizeof(struct blk_copy_payload) +
> + nr_srcs * sizeof(struct range_entry),
> + GFP_ATOMIC | __GFP_NOWARN);
Please check if the use of struct_size() is possible. Probably even assign
total_size here so you don't need to do the size calculation twice.
> + if (!payload)
> + return -ENOMEM;
> +
> + bio = bio_alloc(gfp_mask, 1);
> + bio->bi_iter.bi_sector = dest;
> + bio_set_dev(bio, bdev);
> + bio_set_op_attrs(bio, op, REQ_NOMERGE);
bio_set_op_attrs() is deprecated, please don't use it.
bio->bi_opf = REQ_OP_COPY | REQ_NOMERGE;
> +
> + payload->dest = dest;
> +
> + for (i = 0; i < nr_srcs; i++) {
> + /* copy payload provided are in bytes */
> + src_sects = rlist[i].src;
> + if (src_sects & bs_mask)
> + return -EINVAL;
> + src_sects = src_sects >> SECTOR_SHIFT;
> +
> + if (len & bs_mask)
> + return -EINVAL;
> + len = rlist[i].len >> SECTOR_SHIFT;
> + if (len > q->limits.max_copy_range_sectors)
> + return -EINVAL;
> +
> + total_len += len;
> +
> + WARN_ON_ONCE((src_sects << 9) > UINT_MAX);
> +
> + payload->range[i].src = src_sects;
> + payload->range[i].len = len;
> + }
> +
> + /* storing # of source ranges */
> + payload->copy_range = i;
> + /* storing copy len so far */
> + payload->copy_size = total_len;
> +
> + total_size = sizeof(struct blk_copy_payload) + nr_srcs * sizeof(struct range_entry);
See above.
> + ret = bio_add_page(bio, virt_to_page(payload), total_size,
> + offset_in_page(payload));
> + if (ret != total_size) {
> + kfree(payload);
> + return -ENOMEM;
> + }
> +
> + *biop = bio;
> + return 0;
> +}
> +EXPORT_SYMBOL(__blkdev_issue_copy);
> +
> +/**
> + * blkdev_issue_copy - queue a copy
> + * @bdev: blockdev to issue copy for
> + * @dest: dest sector
> + * @nr_srcs: number of source ranges to copy
> + * @rlist: list of range entries
> + * @gfp_mask: memory allocation flags (for bio_alloc)
> + * @flags: BLKDEV_COPY_* flags to control behaviour //TODO
> + *
> + * Description:
> + * Issue a copy request for dest sector with source in rlist
> + */
> +int blkdev_issue_copy(struct block_device *bdev, sector_t dest,
> + int nr_srcs, struct range_entry *rlist,
> + gfp_t gfp_mask, unsigned long flags)
> +{
> + struct bio *bio = NULL;
> + int ret;
> +
> + ret = __blkdev_issue_copy(bdev, dest, nr_srcs, rlist, gfp_mask, flags,
> + &bio);
> + if (!ret && bio) {
> + ret = submit_bio_wait(bio);
> + if (ret == -EOPNOTSUPP)
> + ret = 0;
> +
> + kfree(page_address(bio_first_bvec_all(bio)->bv_page) +
> + bio_first_bvec_all(bio)->bv_offset);
> + bio_put(bio);
> + }
> +
> + return ret;
What happens with bio here if __blkdev_issue_copy() returns say -ENOMEM because
bio_add_page() fails?
Also please handle failure not success. Sth along the lines of
ret = __blkdev_issue_copy(bdev, dest, nr_srcs, rlist, gfp_mask, flags,
&bio);
if (ret)
...
ret = submit_bio_wait();
if (ret)
...
...
return ret;
}
> +}
> +EXPORT_SYMBOL(blkdev_issue_copy);
Powered by blists - more mailing lists