* Re: [PATCH 02/13] nvme-multipath: add error handling support for add_disk()
From: Keith Busch @ 2021-10-16 0:01 UTC (permalink / raw)
To: Luis Chamberlain
Cc: nvdimm, vigneshr, linux-nvme, paulus, miquel.raynal, ira.weiny,
hch, dave.jiang, sagi, minchan, vishal.l.verma, ngupta,
linux-block, dan.j.williams, axboe, geoff, linux-kernel, jim,
senozhatsky, richard, linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-3-mcgrof@kernel.org>
On Fri, Oct 15, 2021 at 04:52:08PM -0700, Luis Chamberlain wrote:
> We never checked for errors on add_disk() as this function
> returned void. Now that this is fixed, use the shiny new
> error handling.
>
> Since we now can tell for sure when a disk was added, move
> setting the bit NVME_NSHEAD_DISK_LIVE only when we did
> add the disk successfully.
>
> Nothing to do here as the cleanup is done elsewhere. We take
> care and use test_and_set_bit() because it is protects against
> two nvme paths simultaneously calling device_add_disk() on the
> same namespace head.
Looks good, thank you.
Reviewed-by: Keith Busch <kbusch@kernel.org>
^ permalink raw reply
* [PATCH 08/13] zram: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/zram/zram_drv.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 0a9309a2ef54..bdbded107e6b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1983,7 +1983,9 @@ static int zram_add(void)
blk_queue_max_write_zeroes_sectors(zram->disk->queue, UINT_MAX);
blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue);
- device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
+ ret = device_add_disk(NULL, zram->disk, zram_disk_attr_groups);
+ if (ret)
+ goto out_cleanup_disk;
strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
@@ -1991,6 +1993,8 @@ static int zram_add(void)
pr_info("Added device: %s\n", zram->disk->disk_name);
return device_id;
+out_cleanup_disk:
+ blk_cleanup_disk(zram->disk);
out_free_idr:
idr_remove(&zram_index_idr, device_id);
out_free_dev:
--
2.30.2
^ permalink raw reply related
* [PATCH 11/13] ps3vram: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/ps3vram.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
index c7b19e128b03..af2a0d09c598 100644
--- a/drivers/block/ps3vram.c
+++ b/drivers/block/ps3vram.c
@@ -755,9 +755,14 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev)
dev_info(&dev->core, "%s: Using %llu MiB of GPU memory\n",
gendisk->disk_name, get_capacity(gendisk) >> 11);
- device_add_disk(&dev->core, gendisk, NULL);
+ error = device_add_disk(&dev->core, gendisk, NULL);
+ if (error)
+ goto out_cleanup_disk;
+
return 0;
+out_cleanup_disk:
+ blk_cleanup_disk(gendisk);
out_cache_cleanup:
remove_proc_entry(DEVICE_NAME, NULL);
ps3vram_cache_cleanup(dev);
--
2.30.2
^ permalink raw reply related
* [PATCH 05/13] nvdimm/btt: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvdimm/btt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 23ee8c005db5..57b921c5fbb5 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1542,7 +1542,9 @@ static int btt_blk_init(struct btt *btt)
}
set_capacity(btt->btt_disk, btt->nlba * btt->sector_size >> 9);
- device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+ rc = device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+ if (rc)
+ goto out_cleanup_disk;
btt->nd_btt->size = btt->nlba * (u64)btt->sector_size;
nvdimm_check_and_set_ro(btt->btt_disk);
--
2.30.2
^ permalink raw reply related
* [PATCH 13/13] mtd/ubi/block: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/mtd/ubi/block.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index e003b4b44ffa..062e6c2c45f5 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -447,12 +447,18 @@ int ubiblock_create(struct ubi_volume_info *vi)
list_add_tail(&dev->list, &ubiblock_devices);
/* Must be the last step: anyone can call file ops from now on */
- add_disk(dev->gd);
+ ret = add_disk(dev->gd);
+ if (ret)
+ goto out_destroy_wq;
+
dev_info(disk_to_dev(dev->gd), "created from ubi%d:%d(%s)",
dev->ubi_num, dev->vol_id, vi->name);
mutex_unlock(&devices_mutex);
return 0;
+out_destroy_wq:
+ list_del(&dev->list);
+ destroy_workqueue(dev->wq);
out_remove_minor:
idr_remove(&ubiblock_minor_idr, gd->first_minor);
out_cleanup_disk:
--
2.30.2
^ permalink raw reply related
* [PATCH 09/13] z2ram: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling. Only the disk is cleaned up inside
z2ram_register_disk() as the caller deals with the rest.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/z2ram.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/block/z2ram.c b/drivers/block/z2ram.c
index 4eef218108c6..ccc52c935faf 100644
--- a/drivers/block/z2ram.c
+++ b/drivers/block/z2ram.c
@@ -318,6 +318,7 @@ static const struct blk_mq_ops z2_mq_ops = {
static int z2ram_register_disk(int minor)
{
struct gendisk *disk;
+ int err;
disk = blk_mq_alloc_disk(&tag_set, NULL);
if (IS_ERR(disk))
@@ -333,8 +334,10 @@ static int z2ram_register_disk(int minor)
sprintf(disk->disk_name, "z2ram");
z2ram_gendisk[minor] = disk;
- add_disk(disk);
- return 0;
+ err = add_disk(disk);
+ if (err)
+ blk_cleanup_disk(disk);
+ return err;
}
static int __init z2_init(void)
--
2.30.2
^ permalink raw reply related
* [PATCH 06/13] nvdimm/blk: avoid calling del_gendisk() on early failures
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
If nd_integrity_init() fails we'd get del_gendisk() called,
but that's not correct as we should only call that if we're
done with device_add_disk(). Fix this by providing unwinding
prior to the devm call being registered and moving the devm
registration to the very end.
This should fix calling del_gendisk() if nd_integrity_init()
fails. I only spotted this issue through code inspection. It
does not fix any real world bug.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvdimm/blk.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c
index 088d3dd6f6fa..591fa1f86f1e 100644
--- a/drivers/nvdimm/blk.c
+++ b/drivers/nvdimm/blk.c
@@ -240,6 +240,7 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
resource_size_t available_disk_size;
struct gendisk *disk;
u64 internal_nlba;
+ int rc;
internal_nlba = div_u64(nsblk->size, nsblk_internal_lbasize(nsblk));
available_disk_size = internal_nlba * nsblk_sector_size(nsblk);
@@ -256,20 +257,26 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
blk_queue_logical_block_size(disk->queue, nsblk_sector_size(nsblk));
blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
- if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
- return -ENOMEM;
-
if (nsblk_meta_size(nsblk)) {
- int rc = nd_integrity_init(disk, nsblk_meta_size(nsblk));
+ rc = nd_integrity_init(disk, nsblk_meta_size(nsblk));
if (rc)
- return rc;
+ goto out_before_devm_err;
}
set_capacity(disk, available_disk_size >> SECTOR_SHIFT);
device_add_disk(dev, disk, NULL);
+
+ /* nd_blk_release_disk() is called if this fails */
+ if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
+ return -ENOMEM;
+
nvdimm_check_and_set_ro(disk);
return 0;
+
+out_before_devm_err:
+ blk_cleanup_disk(disk);
+ return rc;
}
static int nd_blk_probe(struct device *dev)
--
2.30.2
^ permalink raw reply related
* [PATCH 10/13] ps3disk: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/ps3disk.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index 8d51efbe045d..3054adf77460 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -467,9 +467,13 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev)
gendisk->disk_name, priv->model, priv->raw_capacity >> 11,
get_capacity(gendisk) >> 11);
- device_add_disk(&dev->sbd.core, gendisk, NULL);
- return 0;
+ error = device_add_disk(&dev->sbd.core, gendisk, NULL);
+ if (error)
+ goto fail_cleanup_disk;
+ return 0;
+fail_cleanup_disk:
+ blk_cleanup_disk(gendisk);
fail_free_tag_set:
blk_mq_free_tag_set(&priv->tag_set);
fail_teardown:
--
2.30.2
^ permalink raw reply related
* [PATCH 03/13] nvdimm/btt: do not call del_gendisk() if not needed
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We know we don't need del_gendisk() if we haven't added
the disk, so just skip it. This should fix a bug on older
kernels, as del_gendisk() became able to deal with
disks not added only recently, after the patch titled
"block: add flag for add_disk() completion notation".
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvdimm/btt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 52de60b7adee..29cc7325e890 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1538,7 +1538,6 @@ static int btt_blk_init(struct btt *btt)
int rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
if (rc) {
- del_gendisk(btt->btt_disk);
blk_cleanup_disk(btt->btt_disk);
return rc;
}
--
2.30.2
^ permalink raw reply related
* [PATCH 02/13] nvme-multipath: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Since we now can tell for sure when a disk was added, move
setting the bit NVME_NSHEAD_DISK_LIVE only when we did
add the disk successfully.
Nothing to do here as the cleanup is done elsewhere. We take
care and use test_and_set_bit() because it is protects against
two nvme paths simultaneously calling device_add_disk() on the
same namespace head.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvme/host/multipath.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e8ccdd398f78..022837f7be41 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -496,13 +496,23 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
static void nvme_mpath_set_live(struct nvme_ns *ns)
{
struct nvme_ns_head *head = ns->head;
+ int rc;
if (!head->disk)
return;
+ /*
+ * test_and_set_bit() is used because it is protecting against two nvme
+ * paths simultaneously calling device_add_disk() on the same namespace
+ * head.
+ */
if (!test_and_set_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
- device_add_disk(&head->subsys->dev, head->disk,
- nvme_ns_id_attr_groups);
+ rc = device_add_disk(&head->subsys->dev, head->disk,
+ nvme_ns_id_attr_groups);
+ if (rc) {
+ clear_bit(NVME_NSHEAD_DISK_LIVE, &ns->flags);
+ return;
+ }
nvme_add_ns_head_cdev(head);
}
--
2.30.2
^ permalink raw reply related
* [PATCH 00/13] block: add_disk() error handling stragglers
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
This patch set consists of al the straggler drivers for which we have
have no patch reviews done for yet. I'd like to ask for folks to please
consider chiming in, specially if you're the maintainer for the driver.
Additionally if you can specify if you'll take the patch in yourself or
if you want Jens to take it, that'd be great too.
Luis Chamberlain (13):
block/brd: add error handling support for add_disk()
nvme-multipath: add error handling support for add_disk()
nvdimm/btt: do not call del_gendisk() if not needed
nvdimm/btt: use goto error labels on btt_blk_init()
nvdimm/btt: add error handling support for add_disk()
nvdimm/blk: avoid calling del_gendisk() on early failures
nvdimm/blk: add error handling support for add_disk()
zram: add error handling support for add_disk()
z2ram: add error handling support for add_disk()
ps3disk: add error handling support for add_disk()
ps3vram: add error handling support for add_disk()
block/sunvdc: add error handling support for add_disk()
mtd/ubi/block: add error handling support for add_disk()
drivers/block/brd.c | 9 +++++++--
drivers/block/ps3disk.c | 8 ++++++--
drivers/block/ps3vram.c | 7 ++++++-
drivers/block/sunvdc.c | 14 +++++++++++---
drivers/block/z2ram.c | 7 +++++--
drivers/block/zram/zram_drv.c | 6 +++++-
drivers/mtd/ubi/block.c | 8 +++++++-
drivers/nvdimm/blk.c | 21 +++++++++++++++------
drivers/nvdimm/btt.c | 24 +++++++++++++++---------
drivers/nvme/host/multipath.c | 14 ++++++++++++--
10 files changed, 89 insertions(+), 29 deletions(-)
--
2.30.2
^ permalink raw reply
* [PATCH 07/13] nvdimm/blk: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Since nvdimm/blk uses devm we just need to move the devm
registration towards the end. And in hindsight, that seems
to also provide a fix given del_gendisk() should not be
called unless the disk was already added via add_disk().
The probably of that issue happening is low though, like
OOM while calling devm_add_action(), so the fix is minor.
We manually unwind in case of add_disk() failure prior
to the devm registration.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvdimm/blk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c
index 591fa1f86f1e..9f1eb41404ac 100644
--- a/drivers/nvdimm/blk.c
+++ b/drivers/nvdimm/blk.c
@@ -265,7 +265,9 @@ static int nsblk_attach_disk(struct nd_namespace_blk *nsblk)
}
set_capacity(disk, available_disk_size >> SECTOR_SHIFT);
- device_add_disk(dev, disk, NULL);
+ rc = device_add_disk(dev, disk, NULL);
+ if (rc)
+ goto out_before_devm_err;
/* nd_blk_release_disk() is called if this fails */
if (devm_add_action_or_reset(dev, nd_blk_release_disk, disk))
--
2.30.2
^ permalink raw reply related
* [PATCH 01/13] block/brd: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/brd.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index 530b31240203..6065f493876f 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -372,6 +372,7 @@ static int brd_alloc(int i)
struct brd_device *brd;
struct gendisk *disk;
char buf[DISK_NAME_LEN];
+ int err = -ENOMEM;
mutex_lock(&brd_devices_mutex);
list_for_each_entry(brd, &brd_devices, brd_list) {
@@ -422,16 +423,20 @@ static int brd_alloc(int i)
/* Tell the block layer that this is not a rotational device */
blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue);
blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue);
- add_disk(disk);
+ err = add_disk(disk);
+ if (err)
+ goto out_cleanup_disk;
return 0;
+out_cleanup_disk:
+ blk_cleanup_disk(disk);
out_free_dev:
mutex_lock(&brd_devices_mutex);
list_del(&brd->brd_list);
mutex_unlock(&brd_devices_mutex);
kfree(brd);
- return -ENOMEM;
+ return err;
}
static void brd_probe(dev_t dev)
--
2.30.2
^ permalink raw reply related
* [PATCH 04/13] nvdimm/btt: use goto error labels on btt_blk_init()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
This will make it easier to share common error paths.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/nvdimm/btt.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 29cc7325e890..23ee8c005db5 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1520,10 +1520,11 @@ static int btt_blk_init(struct btt *btt)
{
struct nd_btt *nd_btt = btt->nd_btt;
struct nd_namespace_common *ndns = nd_btt->ndns;
+ int rc = -ENOMEM;
btt->btt_disk = blk_alloc_disk(NUMA_NO_NODE);
if (!btt->btt_disk)
- return -ENOMEM;
+ goto out;
nvdimm_namespace_disk_name(ndns, btt->btt_disk->disk_name);
btt->btt_disk->first_minor = 0;
@@ -1535,19 +1536,23 @@ static int btt_blk_init(struct btt *btt)
blk_queue_flag_set(QUEUE_FLAG_NONROT, btt->btt_disk->queue);
if (btt_meta_size(btt)) {
- int rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
-
- if (rc) {
- blk_cleanup_disk(btt->btt_disk);
- return rc;
- }
+ rc = nd_integrity_init(btt->btt_disk, btt_meta_size(btt));
+ if (rc)
+ goto out_cleanup_disk;
}
+
set_capacity(btt->btt_disk, btt->nlba * btt->sector_size >> 9);
device_add_disk(&btt->nd_btt->dev, btt->btt_disk, NULL);
+
btt->nd_btt->size = btt->nlba * (u64)btt->sector_size;
nvdimm_check_and_set_ro(btt->btt_disk);
return 0;
+
+out_cleanup_disk:
+ blk_cleanup_disk(btt->btt_disk);
+out:
+ return rc;
}
static void btt_blk_cleanup(struct btt *btt)
--
2.30.2
^ permalink raw reply related
* [PATCH 12/13] block/sunvdc: add error handling support for add_disk()
From: Luis Chamberlain @ 2021-10-15 23:52 UTC (permalink / raw)
To: axboe, geoff, mpe, benh, paulus, jim, minchan, ngupta,
senozhatsky, richard, miquel.raynal, vigneshr, dan.j.williams,
vishal.l.verma, dave.jiang, ira.weiny, kbusch, hch, sagi
Cc: nvdimm, linux-kernel, linux-nvme, linux-block, Luis Chamberlain,
linux-mtd, linuxppc-dev
In-Reply-To: <20211015235219.2191207-1-mcgrof@kernel.org>
We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.
We re-use the same free tag call, so we also add a label for
that as well.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
drivers/block/sunvdc.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/block/sunvdc.c b/drivers/block/sunvdc.c
index 4d4bb810c2ae..6f45a53f7cbf 100644
--- a/drivers/block/sunvdc.c
+++ b/drivers/block/sunvdc.c
@@ -826,8 +826,8 @@ static int probe_disk(struct vdc_port *port)
if (IS_ERR(g)) {
printk(KERN_ERR PFX "%s: Could not allocate gendisk.\n",
port->vio.name);
- blk_mq_free_tag_set(&port->tag_set);
- return PTR_ERR(g);
+ err = PTR_ERR(g);
+ goto out_free_tag;
}
port->disk = g;
@@ -879,9 +879,17 @@ static int probe_disk(struct vdc_port *port)
port->vdisk_size, (port->vdisk_size >> (20 - 9)),
port->vio.ver.major, port->vio.ver.minor);
- device_add_disk(&port->vio.vdev->dev, g, NULL);
+ err = device_add_disk(&port->vio.vdev->dev, g, NULL);
+ if (err)
+ goto out_cleanup_disk;
return 0;
+
+out_cleanup_disk:
+ blk_cleanup_disk(g);
+out_free_tag:
+ blk_mq_free_tag_set(&port->tag_set);
+ return err;
}
static struct ldc_channel_config vdc_ldc_cfg = {
--
2.30.2
^ permalink raw reply related
* Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
From: Andy Shevchenko @ 2021-10-15 19:52 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Giovanni Cabiddu, Mark Rutland, Sathya Prakash,
Alexander Shishkin, Alexander Duyck,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), qat-linux,
oss-drivers, Oliver O'Halloran, H. Peter Anvin, Jiri Olsa,
Thomas Gleixner, Marco Chiappero, Stefano Stabellini, Herbert Xu,
linux-scsi, Rafał Miłecki, Jesse Brandeburg,
Peter Zijlstra, Ingo Molnar, linux-pci,
open list:TI WILINK WIRELES..., Jakub Kicinski, Yisen Zhuang,
Suganath Prabu Subramani, Fiona Trahe, Andrew Donnellan,
Arnd Bergmann, Konrad Rzeszutek Wilk, Ido Schimmel,
Uwe Kleine-König, Simon Horman,
open list:LINUX FOR POWERPC PA SEMI PWRFICIENT,
Arnaldo Carvalho de Melo, Jack Xu, Borislav Petkov,
Michael Buesch, Jiri Pirko, Bjorn Helgaas, Namhyung Kim,
Boris Ostrovsky, Juergen Gross, Salil Mehta, Sreekanth Reddy,
xen-devel, Vadym Kochan, MPT-FusionLinux.pdl, Greg Kroah-Hartman,
USB, Wojciech Ziemba, Linux Kernel Mailing List, Mathias Nyman,
Zhou Wang, linux-crypto, Sascha Hauer, netdev, Frederic Barrat,
Paul Mackerras, Tomaszx Kowalik, Taras Chornyi, David S. Miller,
linux-perf-users
In-Reply-To: <20211015164653.GA2108651@bhelgaas>
On Fri, Oct 15, 2021 at 7:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:
...
> so compared to Uwe's v6, I restored that section to the original code.
> My goal here was to make the patch as simple and easy to review as
> possible.
Thanks for elaboration. I have got it.
...
> You're right, this didn't make much sense in that patch. I moved the
> line join to the previous patch, which unindented this section and
> made space for this to fit on one line. Here's the revised commit:
>
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287
Side remark: default without break or return is error prone (okay, to
some extent). Perhaps adding the return statement there will make
things robust and clean.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver
From: Bjorn Helgaas @ 2021-10-15 16:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Giovanni Cabiddu, Mark Rutland, Sathya Prakash,
Alexander Shishkin, Alexander Duyck,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), qat-linux,
oss-drivers, Oliver O'Halloran, H. Peter Anvin, Jiri Olsa,
Thomas Gleixner, Marco Chiappero, Stefano Stabellini, Herbert Xu,
linux-scsi, Rafał Miłecki, Jesse Brandeburg,
Peter Zijlstra, Ingo Molnar, linux-pci,
open list:TI WILINK WIRELES..., Jakub Kicinski, Yisen Zhuang,
Suganath Prabu Subramani, Fiona Trahe, Andrew Donnellan,
Arnd Bergmann, Konrad Rzeszutek Wilk, Ido Schimmel,
Uwe Kleine-König, Simon Horman,
open list:LINUX FOR POWERPC PA SEMI PWRFICIENT,
Arnaldo Carvalho de Melo, Jack Xu, Borislav Petkov,
Michael Buesch, Jiri Pirko, Bjorn Helgaas, Namhyung Kim,
Boris Ostrovsky, Juergen Gross, Salil Mehta, Sreekanth Reddy,
xen-devel, Vadym Kochan, MPT-FusionLinux.pdl, Greg Kroah-Hartman,
USB, Wojciech Ziemba, Linux Kernel Mailing List, Mathias Nyman,
Zhou Wang, linux-crypto, Sascha Hauer, netdev, Frederic Barrat,
Paul Mackerras, Tomaszx Kowalik, Taras Chornyi, David S. Miller,
linux-perf-users
In-Reply-To: <YWbdvc7EWEZLVTHM@smile.fi.intel.com>
On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:
> > > > + return drv && drv->resume ?
> > > > + drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
> > >
> > > One line?
> >
> > I don't think I touched that line.
>
> Then why they are both in + section?
They're both in the + section of the interdiff because Uwe's v6 patch
looks like this:
static int pci_legacy_resume(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- return drv && drv->resume ?
- drv->resume(pci_dev) : pci_pm_reenable_device(pci_dev);
+ if (pci_dev->dev.driver) {
+ struct pci_driver *drv = to_pci_driver(pci_dev->dev.driver);
+
+ if (drv->resume)
+ return drv->resume(pci_dev);
+ }
+
+ return pci_pm_reenable_device(pci_dev);
and my revision looks like this:
static int pci_legacy_resume(struct device *dev)
{
struct pci_dev *pci_dev = to_pci_dev(dev);
- struct pci_driver *drv = pci_dev->driver;
+ struct pci_driver *drv = to_pci_driver(dev->driver);
so compared to Uwe's v6, I restored that section to the original code.
My goal here was to make the patch as simple and easy to review as
possible.
> > > > + struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> > > > const struct pci_error_handlers *err_handler =
> > > > - dev->dev.driver ? to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > > > + drv ? drv->err_handler : NULL;
> > >
> > > Isn't dev->driver == to_pci_driver(dev->dev.driver)?
> >
> > Yes, I think so, but not sure what you're getting at here, can you
> > elaborate?
>
> Getting pointer from another pointer seems waste of resources, why we
> can't simply
>
> struct pci_driver *drv = dev->driver;
I think this is in pci_dev_save_and_disable(), and "dev" here is a
struct pci_dev *. We're removing the dev->driver member. Let me know
if I'm still missing something.
> > > > - "bad request in aer recovery "
> > > > - "operation!\n");
> > > > + "bad request in AER recovery operation!\n");
> > > Stray change? Or is it in a separate patch in your tree?
> >
> > Could be skipped. The string now fits on one line so I combined it to
> > make it more greppable.
>
> This is inconsistency in your changes, in one case you are objecting of
> doing something close to the changed lines, in the other you are doing
> unrelated change.
You're right, this didn't make much sense in that patch. I moved the
line join to the previous patch, which unindented this section and
made space for this to fit on one line. Here's the revised commit:
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287
^ permalink raw reply
* RE: [PATCH] soc: fsl: dpio: protect smp_processor_id when get processor id
From: Li, Meng @ 2021-10-15 16:37 UTC (permalink / raw)
To: Robin Murphy, Roy.Pledge@nxp.com, leoyang.li@nxp.com,
ruxandra.radulescu@nxp.com, horia.geanta@nxp.com
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <ba116805-8e41-1502-6cd4-acc1d5e5fa41@arm.com>
> -----Original Message-----
> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, October 15, 2021 9:40 PM
> To: Li, Meng <Meng.Li@windriver.com>; Roy.Pledge@nxp.com;
> leoyang.li@nxp.com; ruxandra.radulescu@nxp.com; horia.geanta@nxp.com
> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH] soc: fsl: dpio: protect smp_processor_id when get
> processor id
>
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 2021-10-15 07:36, Meng.Li@windriver.com wrote:
> > From: Meng Li <meng.li@windriver.com>
> >
> > When enable debug kernel configs,there will be calltrace as below:
> >
> > BUG: using smp_processor_id() in preemptible [00000000] code:
> > swapper/0/1 caller is debug_smp_processor_id+0x20/0x30
> > CPU: 6 PID: 1 Comm: swapper/0 Not tainted 5.10.63-yocto-standard #1
> > Hardware name: NXP Layerscape LX2160ARDB (DT) Call trace:
> > dump_backtrace+0x0/0x1a0
> > show_stack+0x24/0x30
> > dump_stack+0xf0/0x13c
> > check_preemption_disabled+0x100/0x110
> > debug_smp_processor_id+0x20/0x30
> > dpaa2_io_query_fq_count+0xdc/0x154
> > dpaa2_eth_stop+0x144/0x314
> > __dev_close_many+0xdc/0x160
> > __dev_change_flags+0xe8/0x220
> > dev_change_flags+0x30/0x70
> > ic_close_devs+0x50/0x78
> > ip_auto_config+0xed0/0xf10
> > do_one_initcall+0xac/0x460
> > kernel_init_freeable+0x30c/0x378
> > kernel_init+0x20/0x128
> > ret_from_fork+0x10/0x38
> >
> > Because smp_processor_id() should be invoked in preempt disable status.
> > So, add preempt_disable/enable() to protect smp_processor_id().
>
> If preemption doesn't matter anyway, as the comment in the context implies,
> then it probably makes more sense just to use
> raw_smp_processor_id() instead.
>
Thanks for your professional suggest, I will raw_smp_processor_id() and test.
If works fine, I will send v2 patch.
Thanks,
Limeng
> Robin.
>
> > Fixes: c89105c9b390 ("staging: fsl-mc: Move DPIO from staging to
> > drivers/soc/fsl")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Meng Li <Meng.Li@windriver.com>
> > ---
> > drivers/soc/fsl/dpio/dpio-service.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/soc/fsl/dpio/dpio-service.c
> > b/drivers/soc/fsl/dpio/dpio-service.c
> > index 19f47ea9dab0..afc3b89b0fc5 100644
> > --- a/drivers/soc/fsl/dpio/dpio-service.c
> > +++ b/drivers/soc/fsl/dpio/dpio-service.c
> > @@ -58,8 +58,11 @@ static inline struct dpaa2_io
> *service_select_by_cpu(struct dpaa2_io *d,
> > * If cpu == -1, choose the current cpu, with no guarantees about
> > * potentially being migrated away.
> > */
> > - if (cpu < 0)
> > - cpu = smp_processor_id();
> > + if (cpu < 0) {
> > + preempt_disable();
> > + cpu = smp_processor_id();
> > + preempt_enable();
> > + }
> >
> > /* If a specific cpu was requested, pick it up immediately */
> > return dpio_by_cpu[cpu];
> >
^ permalink raw reply
* Re: [PATCH 2/2] sched: Centralize SCHED_{SMT, MC, CLUSTER} definitions
From: Peter Zijlstra @ 2021-10-15 13:04 UTC (permalink / raw)
To: Valentin Schneider
Cc: Juri Lelli, Mark Rutland, Kefeng Wang, Rich Felker, linux-ia64,
Geert Uytterhoeven, linux-sh, Catalin Marinas, Linus Walleij,
David Hildenbrand, x86, linux-mips, James E.J. Bottomley,
Hugh Dickins, Paul Mackerras, H. Peter Anvin, sparclinux,
Will Deacon, Ard Biesheuvel, linux-s390, Vincent Guittot,
Arnd Bergmann, Yoshinori Sato, YiFei Zhu, Helge Deller, Aubrey Li,
Barry Song, Russell King, Christian Borntraeger, Ingo Molnar,
Mel Gorman, Masahiro Yamada, Frederic Weisbecker, Kees Cook,
Vasily Gorbik, Anshuman Khandual, Vlastimil Babka, Vipin Sharma,
Heiko Carstens, Uwe Kleine-König, Steven Rostedt,
Nathan Chancellor, Borislav Petkov, Sergei Trofimovich,
Jonathan Cameron, Thomas Gleixner, Michal Hocko, Dietmar Eggemann,
LAK, Barry Song, Ben Segall, Thomas Bogendoerfer, Daniel Borkmann,
linux-parisc, Chris Down, linuxppc-dev, Randy Dunlap,
Nick Desaulniers, LKML, Rasmus Villemoes,
Daniel Bristot de Oliveira, Andrew Morton, Tim Chen,
David S. Miller, Mike Rapoport
In-Reply-To: <87bl3zlex8.mognet@arm.com>
On Fri, Oct 08, 2021 at 04:22:27PM +0100, Valentin Schneider wrote:
> So x86 has it default yes, and a lot of others (e.g. arm64) have it default
> no.
>
> IMO you don't gain much by disabling them. SCHED_MC and SCHED_CLUSTER only
> control the presence of a sched_domain_topology_level - if it's useless it
> gets degenerated at domain build time. Some valid reasons for not using
> them is if the architecture defines its own topology table (e.g. powerpc
> has CACHE and MC levels which are not gated behind any CONFIG).
>
> SCHED_SMT has an impact on code generated in sched/core.c, but that is also
> gated by a static key.
>
> So I'd say having them default yes is sensible. I'd even say we should
> change the "If unsure say N here." to "Y".
Right, so I tend to agree (and also that we should fix that Kconfig help
text). But it would be very nice to have feedback from the affected arch
maintainers.
^ permalink raw reply
* [PATCH] soc: fsl: dpio: protect smp_processor_id when get processor id
From: Meng.Li @ 2021-10-15 6:36 UTC (permalink / raw)
To: Roy.Pledge, leoyang.li, ruxandra.radulescu, horia.geanta
Cc: meng.li, linuxppc-dev, linux-kernel, linux-arm-kernel
From: Meng Li <meng.li@windriver.com>
When enable debug kernel configs,there will be calltrace as below:
BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
caller is debug_smp_processor_id+0x20/0x30
CPU: 6 PID: 1 Comm: swapper/0 Not tainted 5.10.63-yocto-standard #1
Hardware name: NXP Layerscape LX2160ARDB (DT)
Call trace:
dump_backtrace+0x0/0x1a0
show_stack+0x24/0x30
dump_stack+0xf0/0x13c
check_preemption_disabled+0x100/0x110
debug_smp_processor_id+0x20/0x30
dpaa2_io_query_fq_count+0xdc/0x154
dpaa2_eth_stop+0x144/0x314
__dev_close_many+0xdc/0x160
__dev_change_flags+0xe8/0x220
dev_change_flags+0x30/0x70
ic_close_devs+0x50/0x78
ip_auto_config+0xed0/0xf10
do_one_initcall+0xac/0x460
kernel_init_freeable+0x30c/0x378
kernel_init+0x20/0x128
ret_from_fork+0x10/0x38
Because smp_processor_id() should be invoked in preempt disable status.
So, add preempt_disable/enable() to protect smp_processor_id().
Fixes: c89105c9b390 ("staging: fsl-mc: Move DPIO from staging to drivers/soc/fsl")
Cc: stable@vger.kernel.org
Signed-off-by: Meng Li <Meng.Li@windriver.com>
---
drivers/soc/fsl/dpio/dpio-service.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/dpio/dpio-service.c b/drivers/soc/fsl/dpio/dpio-service.c
index 19f47ea9dab0..afc3b89b0fc5 100644
--- a/drivers/soc/fsl/dpio/dpio-service.c
+++ b/drivers/soc/fsl/dpio/dpio-service.c
@@ -58,8 +58,11 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
* If cpu == -1, choose the current cpu, with no guarantees about
* potentially being migrated away.
*/
- if (cpu < 0)
- cpu = smp_processor_id();
+ if (cpu < 0) {
+ preempt_disable();
+ cpu = smp_processor_id();
+ preempt_enable();
+ }
/* If a specific cpu was requested, pick it up immediately */
return dpio_by_cpu[cpu];
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 11/12] ethernet: ibmveth: use ether_addr_to_u64()
From: Tyrel Datwyler @ 2021-10-15 22:28 UTC (permalink / raw)
To: Jakub Kicinski, davem; +Cc: cforno12, paulus, linuxppc-dev, netdev
In-Reply-To: <20211015221652.827253-12-kuba@kernel.org>
On 10/15/21 3:16 PM, Jakub Kicinski wrote:
> We'll want to make netdev->dev_addr const, remove the local
> helper which is missing a const qualifier on the argument
> and use ether_addr_to_u64().
LGTM. ibmveth_encode_mac_addr() is clearly code duplication of
ether_addr_to_u64() minus the const qualifier.
Reviewed-by: Tyrel Datwyler <tyreld@linux.ibm.com>
>
> Similar story to mlx4.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: cforno12@linux.ibm.com
> CC: mpe@ellerman.id.au
> CC: benh@kernel.crashing.org
> CC: paulus@samba.org
> CC: linuxppc-dev@lists.ozlabs.org
> ---
> drivers/net/ethernet/ibm/ibmveth.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 836617fb3f40..45ba40cf4d07 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -483,17 +483,6 @@ static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
> return rc;
> }
>
> -static u64 ibmveth_encode_mac_addr(u8 *mac)
> -{
> - int i;
> - u64 encoded = 0;
> -
> - for (i = 0; i < ETH_ALEN; i++)
> - encoded = (encoded << 8) | mac[i];
> -
> - return encoded;
> -}
> -
> static int ibmveth_open(struct net_device *netdev)
> {
> struct ibmveth_adapter *adapter = netdev_priv(netdev);
> @@ -553,7 +542,7 @@ static int ibmveth_open(struct net_device *netdev)
> adapter->rx_queue.num_slots = rxq_entries;
> adapter->rx_queue.toggle = 1;
>
> - mac_address = ibmveth_encode_mac_addr(netdev->dev_addr);
> + mac_address = ether_addr_to_u64(netdev->dev_addr);
>
> rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
> adapter->rx_queue.queue_len;
> @@ -1476,7 +1465,7 @@ static void ibmveth_set_multicast_list(struct net_device *netdev)
> netdev_for_each_mc_addr(ha, netdev) {
> /* add the multicast address to the filter table */
> u64 mcast_addr;
> - mcast_addr = ibmveth_encode_mac_addr(ha->addr);
> + mcast_addr = ether_addr_to_u64(ha->addr);
> lpar_rc = h_multicast_ctrl(adapter->vdev->unit_address,
> IbmVethMcastAddFilter,
> mcast_addr);
> @@ -1606,7 +1595,7 @@ static int ibmveth_set_mac_addr(struct net_device *dev, void *p)
> if (!is_valid_ether_addr(addr->sa_data))
> return -EADDRNOTAVAIL;
>
> - mac_address = ibmveth_encode_mac_addr(addr->sa_data);
> + mac_address = ether_addr_to_u64(addr->sa_data);
> rc = h_change_logical_lan_mac(adapter->vdev->unit_address, mac_address);
> if (rc) {
> netdev_err(adapter->netdev, "h_change_logical_lan_mac failed with rc=%d\n", rc);
>
^ permalink raw reply
* [PATCH net-next 11/12] ethernet: ibmveth: use ether_addr_to_u64()
From: Jakub Kicinski @ 2021-10-15 22:16 UTC (permalink / raw)
To: davem; +Cc: cforno12, netdev, paulus, Jakub Kicinski, linuxppc-dev
In-Reply-To: <20211015221652.827253-1-kuba@kernel.org>
We'll want to make netdev->dev_addr const, remove the local
helper which is missing a const qualifier on the argument
and use ether_addr_to_u64().
Similar story to mlx4.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: cforno12@linux.ibm.com
CC: mpe@ellerman.id.au
CC: benh@kernel.crashing.org
CC: paulus@samba.org
CC: linuxppc-dev@lists.ozlabs.org
---
drivers/net/ethernet/ibm/ibmveth.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 836617fb3f40..45ba40cf4d07 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -483,17 +483,6 @@ static int ibmveth_register_logical_lan(struct ibmveth_adapter *adapter,
return rc;
}
-static u64 ibmveth_encode_mac_addr(u8 *mac)
-{
- int i;
- u64 encoded = 0;
-
- for (i = 0; i < ETH_ALEN; i++)
- encoded = (encoded << 8) | mac[i];
-
- return encoded;
-}
-
static int ibmveth_open(struct net_device *netdev)
{
struct ibmveth_adapter *adapter = netdev_priv(netdev);
@@ -553,7 +542,7 @@ static int ibmveth_open(struct net_device *netdev)
adapter->rx_queue.num_slots = rxq_entries;
adapter->rx_queue.toggle = 1;
- mac_address = ibmveth_encode_mac_addr(netdev->dev_addr);
+ mac_address = ether_addr_to_u64(netdev->dev_addr);
rxq_desc.fields.flags_len = IBMVETH_BUF_VALID |
adapter->rx_queue.queue_len;
@@ -1476,7 +1465,7 @@ static void ibmveth_set_multicast_list(struct net_device *netdev)
netdev_for_each_mc_addr(ha, netdev) {
/* add the multicast address to the filter table */
u64 mcast_addr;
- mcast_addr = ibmveth_encode_mac_addr(ha->addr);
+ mcast_addr = ether_addr_to_u64(ha->addr);
lpar_rc = h_multicast_ctrl(adapter->vdev->unit_address,
IbmVethMcastAddFilter,
mcast_addr);
@@ -1606,7 +1595,7 @@ static int ibmveth_set_mac_addr(struct net_device *dev, void *p)
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
- mac_address = ibmveth_encode_mac_addr(addr->sa_data);
+ mac_address = ether_addr_to_u64(addr->sa_data);
rc = h_change_logical_lan_mac(adapter->vdev->unit_address, mac_address);
if (rc) {
netdev_err(adapter->netdev, "h_change_logical_lan_mac failed with rc=%d\n", rc);
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v2 13/13] lkdtm: Add a test for function descriptors protection
From: Kees Cook @ 2021-10-15 21:35 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <08a3dfbc55e1c7a0a1917b22f0ca6cabd9895ab2.1634190022.git.christophe.leroy@csgroup.eu>
On Thu, Oct 14, 2021 at 07:50:02AM +0200, Christophe Leroy wrote:
> Add WRITE_OPD to check that you can't modify function
> descriptors.
>
> Gives the following result when function descriptors are
> not protected:
>
> lkdtm: Performing direct entry WRITE_OPD
> lkdtm: attempting bad 16 bytes write at c00000000269b358
> lkdtm: FAIL: survived bad write
> lkdtm: do_nothing was hijacked!
>
> Looks like a standard compiler barrier(); is not enough to force
> GCC to use the modified function descriptor. Add to add a fake empty
> inline assembly to force GCC to reload the function descriptor.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/core.c | 1 +
> drivers/misc/lkdtm/lkdtm.h | 1 +
> drivers/misc/lkdtm/perms.c | 22 ++++++++++++++++++++++
> 3 files changed, 24 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index fe6fd34b8caf..de092aa03b5d 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -148,6 +148,7 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(WRITE_RO),
> CRASHTYPE(WRITE_RO_AFTER_INIT),
> CRASHTYPE(WRITE_KERN),
> + CRASHTYPE(WRITE_OPD),
> CRASHTYPE(REFCOUNT_INC_OVERFLOW),
> CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
> CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index c212a253edde..188bd0fd6575 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -105,6 +105,7 @@ void __init lkdtm_perms_init(void);
> void lkdtm_WRITE_RO(void);
> void lkdtm_WRITE_RO_AFTER_INIT(void);
> void lkdtm_WRITE_KERN(void);
> +void lkdtm_WRITE_OPD(void);
> void lkdtm_EXEC_DATA(void);
> void lkdtm_EXEC_STACK(void);
> void lkdtm_EXEC_KMALLOC(void);
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 96b3ebfcb8ed..3870bc82d40d 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,6 +44,11 @@ static noinline void do_overwritten(void)
> return;
> }
>
> +static noinline void do_almost_nothing(void)
> +{
> + pr_info("do_nothing was hijacked!\n");
> +}
> +
> static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
> {
> memcpy(fdesc, do_nothing, sizeof(*fdesc));
> @@ -143,6 +148,23 @@ void lkdtm_WRITE_KERN(void)
> do_overwritten();
> }
>
> +void lkdtm_WRITE_OPD(void)
> +{
> + size_t size = sizeof(func_desc_t);
> + void (*func)(void) = do_nothing;
> +
> + if (!have_function_descriptors()) {
> + pr_info("Platform doesn't have function descriptors.\n");
This should be more explicit ('xfail'):
pr_info("XFAIL: platform doesn't use function descriptors.\n");
> + return;
> + }
> + pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing);
> + memcpy(do_nothing, do_almost_nothing, size);
> + pr_err("FAIL: survived bad write\n");
> +
> + asm("" : "=m"(func));
Since this is a descriptor, I assume no icache flush is needed. Are
function descriptors strictly dcache? (Is anything besides just a
barrier needed?)
> + func();
> +}
> +
> void lkdtm_EXEC_DATA(void)
> {
> execute_location(data_area, CODE_WRITE);
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 11/13] lkdtm: Fix lkdtm_EXEC_RODATA()
From: Kees Cook @ 2021-10-15 21:32 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <44946ed0340013a52f8acdee7d6d0781f145cd6b.1634190022.git.christophe.leroy@csgroup.eu>
On Thu, Oct 14, 2021 at 07:50:00AM +0200, Christophe Leroy wrote:
> Behind its location, lkdtm_EXEC_RODATA() executes
> lkdtm_rodata_do_nothing() which is a real function,
> not a copy of do_nothing().
>
> So executes it directly instead of using execute_location().
>
> This is necessary because following patch will fix execute_location()
> to use a copy of the function descriptor of do_nothing() and
> function descriptor of lkdtm_rodata_do_nothing() might be different.
>
> And fix displayed addresses by dereferencing the function descriptors.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
I still don't understand this -- it doesn't look needed at all given the
changes in patch 12. (i.e. everything is using
dereference_function_descriptor() now)
Can't this patch be dropped?
-Kees
> ---
> drivers/misc/lkdtm/perms.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 035fcca441f0..5266dc28df6e 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -153,7 +153,14 @@ void lkdtm_EXEC_VMALLOC(void)
>
> void lkdtm_EXEC_RODATA(void)
> {
> - execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
> + pr_info("attempting ok execution at %px\n",
> + dereference_function_descriptor(do_nothing));
> + do_nothing();
> +
> + pr_info("attempting bad execution at %px\n",
> + dereference_function_descriptor(lkdtm_rodata_do_nothing));
> + lkdtm_rodata_do_nothing();
> + pr_err("FAIL: func returned\n");
> }
>
> void lkdtm_EXEC_USERSPACE(void)
> --
> 2.31.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 12/13] lkdtm: Fix execute_[user]_location()
From: Kees Cook @ 2021-10-15 21:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, linux-ia64, linux-parisc, Arnd Bergmann,
Greg Kroah-Hartman, Helge Deller, linux-kernel,
James E.J. Bottomley, linux-mm, Paul Mackerras, Andrew Morton,
linuxppc-dev
In-Reply-To: <cbee30c66890994e116a8eae8094fa8c5336f90a.1634190022.git.christophe.leroy@csgroup.eu>
On Thu, Oct 14, 2021 at 07:50:01AM +0200, Christophe Leroy wrote:
> execute_location() and execute_user_location() intent
> to copy do_nothing() text and execute it at a new location.
> However, at the time being it doesn't copy do_nothing() function
> but do_nothing() function descriptor which still points to the
> original text. So at the end it still executes do_nothing() at
> its original location allthough using a copied function descriptor.
>
> So, fix that by really copying do_nothing() text and build a new
> function descriptor by copying do_nothing() function descriptor and
> updating the target address with the new location.
>
> Also fix the displayed addresses by dereferencing do_nothing()
> function descriptor.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> drivers/misc/lkdtm/perms.c | 25 +++++++++++++++++++++----
> include/asm-generic/sections.h | 5 +++++
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 5266dc28df6e..96b3ebfcb8ed 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,19 +44,32 @@ static noinline void do_overwritten(void)
> return;
> }
>
> +static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
> +{
> + memcpy(fdesc, do_nothing, sizeof(*fdesc));
> + fdesc->addr = (unsigned long)dst;
> + barrier();
> +
> + return fdesc;
> +}
How about collapsing the "have_function_descriptors()" check into
setup_function_descriptor()?
static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
{
if (__is_defined(HAVE_FUNCTION_DESCRIPTORS)) {
memcpy(fdesc, do_nothing, sizeof(*fdesc));
fdesc->addr = (unsigned long)dst;
barrier();
return fdesc;
} else {
return dst;
}
}
> +
> static noinline void execute_location(void *dst, bool write)
> {
> void (*func)(void) = dst;
> + func_desc_t fdesc;
> + void *do_nothing_text = dereference_function_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> if (write == CODE_WRITE) {
> - memcpy(dst, do_nothing, EXEC_SIZE);
> + memcpy(dst, do_nothing_text, EXEC_SIZE);
> flush_icache_range((unsigned long)dst,
> (unsigned long)dst + EXEC_SIZE);
> }
> pr_info("attempting bad execution at %px\n", func);
> + if (have_function_descriptors())
> + func = setup_function_descriptor(&fdesc, dst);
> func();
> pr_err("FAIL: func returned\n");
> }
> @@ -67,15 +80,19 @@ static void execute_user_location(void *dst)
>
> /* Intentionally crossing kernel/user memory boundary. */
> void (*func)(void) = dst;
> + func_desc_t fdesc;
> + void *do_nothing_text = dereference_function_descriptor(do_nothing);
>
> - pr_info("attempting ok execution at %px\n", do_nothing);
> + pr_info("attempting ok execution at %px\n", do_nothing_text);
> do_nothing();
>
> - copied = access_process_vm(current, (unsigned long)dst, do_nothing,
> + copied = access_process_vm(current, (unsigned long)dst, do_nothing_text,
> EXEC_SIZE, FOLL_WRITE);
> if (copied < EXEC_SIZE)
> return;
> pr_info("attempting bad execution at %px\n", func);
> + if (have_function_descriptors())
> + func = setup_function_descriptor(&fdesc, dst);
> func();
> pr_err("FAIL: func returned\n");
> }
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 76163883c6ff..d225318538bd 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -70,6 +70,11 @@ typedef struct {
> } func_desc_t;
> #endif
>
> +static inline bool have_function_descriptors(void)
> +{
> + return __is_defined(HAVE_FUNCTION_DESCRIPTORS);
> +}
> +
> /* random extra sections (if any). Override
> * in asm/sections.h */
> #ifndef arch_is_kernel_text
This hunk seems like it should live in a separate patch.
--
Kees Cook
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox