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
| ||
|
Date: Tue, 26 Jul 2022 16:45:44 +0200 From: Jiri Pirko <jiri@...nulli.us> To: Vladimir Oltean <olteanv@...il.com> Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com, andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com Subject: Re: [patch net-next RFC] net: dsa: move port_setup/teardown to be called outside devlink port registered area Tue, Jul 26, 2022 at 03:43:09PM CEST, olteanv@...il.com wrote: >Hi Jiri, > >On Tue, Jul 26, 2022 at 02:41:05PM +0200, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@...dia.com> >> >> Move port_setup() op to be called before devlink_port_register() and >> port_teardown() after devlink_port_unregister(). >> >> RFC note: I don't see why this would not work, but I have no way to >> test this does not break things. But I think it makes sense to move this >> alongside the rest of the devlink port code, the reinit() function >> also gets much nicer, as clearly the fact that >> port_setup()->devlink_port_region_create() was called in dsa_port_setup >> did not fit the flow. >> >> Signed-off-by: Jiri Pirko <jiri@...dia.com> >> --- > >devlink_port->devlink isn't set (it's set in devl_port_register), so >when devlink_port_region_create() calls devl_lock(devlink), it blasts >right through that NULL pointer: > >[ 4.966960] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000320 >[ 5.009201] [0000000000000320] user address but active_mm is swapper >[ 5.015616] Internal error: Oops: 96000004 [#1] PREEMPT SMP >[ 5.024244] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc7-07010-ga9b9500ffaac-dirty #3395 >[ 5.033281] Hardware name: CZ.NIC Turris Mox Board (DT) >[ 5.038499] Workqueue: events_unbound deferred_probe_work_func >[ 5.044342] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) >[ 5.051297] pc : __mutex_lock+0x5c/0x460 >[ 5.055220] lr : __mutex_lock+0x50/0x460 >[ 5.133818] Call trace: >[ 5.136258] __mutex_lock+0x5c/0x460 >[ 5.139831] mutex_lock_nested+0x40/0x50 >[ 5.143750] devlink_port_region_create+0x54/0x15c >[ 5.148542] dsa_devlink_port_region_create+0x64/0x90 >[ 5.153592] mv88e6xxx_setup_devlink_regions_port+0x30/0x60 >[ 5.159165] mv88e6xxx_port_setup+0x10/0x20 >[ 5.163345] dsa_port_devlink_setup+0x60/0x150 >[ 5.167786] dsa_register_switch+0x938/0x119c >[ 5.172138] mv88e6xxx_probe+0x714/0x770 >[ 5.176058] mdio_probe+0x34/0x70 >[ 5.179370] really_probe.part.0+0x9c/0x2ac >[ 5.183550] __driver_probe_device+0x98/0x144 >[ 5.187902] driver_probe_device+0xac/0x14c Oh yes, could you please try together with following patch? (nevermind chicken-egg problem you may spot now) Subject: [patch net-next RFC] net: devlink: convert region creation/destroy() to be forbidden on registered devlink/port No need to create or destroy region when devlink or devlink ports are registered. Limit the possibility to call the region create/destroy() only for non-registered devlink or devlink port. Benefit from that and avoid need to take devl_lock. Signed-off-by: Jiri Pirko <jiri@...dia.com> --- drivers/net/netdevsim/dev.c | 8 ++-- include/net/devlink.h | 5 --- net/core/devlink.c | 78 ++++++++----------------------------- 3 files changed, 20 insertions(+), 71 deletions(-) diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 925dc8a5254d..3f0c19e30650 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -557,15 +557,15 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev, struct devlink *devlink) { nsim_dev->dummy_region = - devl_region_create(devlink, &dummy_region_ops, - NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX, - NSIM_DEV_DUMMY_REGION_SIZE); + devlink_region_create(devlink, &dummy_region_ops, + NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX, + NSIM_DEV_DUMMY_REGION_SIZE); return PTR_ERR_OR_ZERO(nsim_dev->dummy_region); } static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev) { - devl_region_destroy(nsim_dev->dummy_region); + devlink_region_destroy(nsim_dev->dummy_region); } static int diff --git a/include/net/devlink.h b/include/net/devlink.h index 9edb4a28cf30..2416750e050d 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1666,10 +1666,6 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id, int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id, union devlink_param_value init_val); void devlink_param_value_changed(struct devlink *devlink, u32 param_id); -struct devlink_region *devl_region_create(struct devlink *devlink, - const struct devlink_region_ops *ops, - u32 region_max_snapshots, - u64 region_size); struct devlink_region * devlink_region_create(struct devlink *devlink, const struct devlink_region_ops *ops, @@ -1678,7 +1674,6 @@ struct devlink_region * devlink_port_region_create(struct devlink_port *port, const struct devlink_port_region_ops *ops, u32 region_max_snapshots, u64 region_size); -void devl_region_destroy(struct devlink_region *region); void devlink_region_destroy(struct devlink_region *region); void devlink_port_region_destroy(struct devlink_region *region); diff --git a/net/core/devlink.c b/net/core/devlink.c index 4e0c4f9265e8..15d28aba69fc 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -5701,8 +5701,7 @@ static void devlink_nl_region_notify(struct devlink_region *region, struct sk_buff *msg; WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL); - if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED)) - return; + ASSERT_DEVLINK_REGISTERED(devlink); msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0); if (IS_ERR(msg)) @@ -11131,21 +11130,22 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id) EXPORT_SYMBOL_GPL(devlink_param_value_changed); /** - * devl_region_create - create a new address region + * devlink_region_create - create a new address region * * @devlink: devlink * @ops: region operations and name * @region_max_snapshots: Maximum supported number of snapshots for region * @region_size: size of region */ -struct devlink_region *devl_region_create(struct devlink *devlink, - const struct devlink_region_ops *ops, - u32 region_max_snapshots, - u64 region_size) +struct devlink_region * +devlink_region_create(struct devlink *devlink, + const struct devlink_region_ops *ops, + u32 region_max_snapshots, + u64 region_size) { struct devlink_region *region; - devl_assert_locked(devlink); + ASSERT_DEVLINK_NOT_REGISTERED(devlink); if (WARN_ON(!ops) || WARN_ON(!ops->destructor)) return ERR_PTR(-EINVAL); @@ -11164,35 +11164,9 @@ struct devlink_region *devl_region_create(struct devlink *devlink, INIT_LIST_HEAD(®ion->snapshot_list); mutex_init(®ion->snapshot_lock); list_add_tail(®ion->list, &devlink->region_list); - devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW); return region; } -EXPORT_SYMBOL_GPL(devl_region_create); - -/** - * devlink_region_create - create a new address region - * - * @devlink: devlink - * @ops: region operations and name - * @region_max_snapshots: Maximum supported number of snapshots for region - * @region_size: size of region - * - * Context: Takes and release devlink->lock <mutex>. - */ -struct devlink_region * -devlink_region_create(struct devlink *devlink, - const struct devlink_region_ops *ops, - u32 region_max_snapshots, u64 region_size) -{ - struct devlink_region *region; - - devl_lock(devlink); - region = devl_region_create(devlink, ops, region_max_snapshots, - region_size); - devl_unlock(devlink); - return region; -} EXPORT_SYMBOL_GPL(devlink_region_create); /** @@ -11202,8 +11176,6 @@ EXPORT_SYMBOL_GPL(devlink_region_create); * @ops: region operations and name * @region_max_snapshots: Maximum supported number of snapshots for region * @region_size: size of region - * - * Context: Takes and release devlink->lock <mutex>. */ struct devlink_region * devlink_port_region_create(struct devlink_port *port, @@ -11214,11 +11186,11 @@ devlink_port_region_create(struct devlink_port *port, struct devlink_region *region; int err = 0; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(port); + if (WARN_ON(!ops) || WARN_ON(!ops->destructor)) return ERR_PTR(-EINVAL); - devl_lock(devlink); - if (devlink_port_region_get_by_name(port, ops->name)) { err = -EEXIST; goto unlock; @@ -11238,9 +11210,7 @@ devlink_port_region_create(struct devlink_port *port, INIT_LIST_HEAD(®ion->snapshot_list); mutex_init(®ion->snapshot_lock); list_add_tail(®ion->list, &port->region_list); - devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW); - devl_unlock(devlink); return region; unlock: @@ -11250,16 +11220,18 @@ devlink_port_region_create(struct devlink_port *port, EXPORT_SYMBOL_GPL(devlink_port_region_create); /** - * devl_region_destroy - destroy address region + * devlink_region_destroy - destroy address region * * @region: devlink region to destroy */ -void devl_region_destroy(struct devlink_region *region) +void devlink_region_destroy(struct devlink_region *region) { - struct devlink *devlink = region->devlink; struct devlink_snapshot *snapshot, *ts; - devl_assert_locked(devlink); + if (region->port) + ASSERT_DEVLINK_PORT_NOT_REGISTERED(region->port); + else + ASSERT_DEVLINK_NOT_REGISTERED(region->devlink); /* Free all snapshots of region */ list_for_each_entry_safe(snapshot, ts, ®ion->snapshot_list, list) @@ -11268,26 +11240,8 @@ void devl_region_destroy(struct devlink_region *region) list_del(®ion->list); mutex_destroy(®ion->snapshot_lock); - devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL); kfree(region); } -EXPORT_SYMBOL_GPL(devl_region_destroy); - -/** - * devlink_region_destroy - destroy address region - * - * @region: devlink region to destroy - * - * Context: Takes and release devlink->lock <mutex>. - */ -void devlink_region_destroy(struct devlink_region *region) -{ - struct devlink *devlink = region->devlink; - - devl_lock(devlink); - devl_region_destroy(region); - devl_unlock(devlink); -} EXPORT_SYMBOL_GPL(devlink_region_destroy); /** -- 2.35.3
Powered by blists - more mailing lists