diff mbox series

[RFC,net-next,02/12] devlink: Hold a reference on parent device

Message ID 20231017074257.3389177-3-idosch@nvidia.com (mailing list archive)
State Handled Elsewhere
Delegated to: Bjorn Helgaas
Headers show
Series mlxsw: Add support for new reset flow | expand

Commit Message

Ido Schimmel Oct. 17, 2023, 7:42 a.m. UTC
Each devlink instance is associated with a parent device and a pointer
to this device is stored in the devlink structure, but devlink does not
hold a reference on this device.

This is going to be a problem in the next patch where - among other
things - devlink will acquire the device lock during netns dismantle,
before the reload operation. Since netns dismantle is performed
asynchronously and since a reference is not held on the parent device,
it will be possible to hit a use-after-free.

Prepare for the upcoming change by holding a reference on the parent
device.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/core.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jiri Pirko Oct. 17, 2023, 7:56 a.m. UTC | #1
Tue, Oct 17, 2023 at 09:42:47AM CEST, idosch@nvidia.com wrote:
>Each devlink instance is associated with a parent device and a pointer
>to this device is stored in the devlink structure, but devlink does not
>hold a reference on this device.
>
>This is going to be a problem in the next patch where - among other
>things - devlink will acquire the device lock during netns dismantle,
>before the reload operation. Since netns dismantle is performed
>asynchronously and since a reference is not held on the parent device,
>it will be possible to hit a use-after-free.
>
>Prepare for the upcoming change by holding a reference on the parent
>device.
>

Just a note, I'm currently pushing the same patch as a part
of my patchset:
https://lore.kernel.org/all/20231013121029.353351-4-jiri@resnulli.us/


>Signed-off-by: Ido Schimmel <idosch@nvidia.com>
>Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>---
> net/devlink/core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/net/devlink/core.c b/net/devlink/core.c
>index bcbbb952569f..5b8b692b8c76 100644
>--- a/net/devlink/core.c
>+++ b/net/devlink/core.c
>@@ -4,6 +4,7 @@
>  * Copyright (c) 2016 Jiri Pirko <jiri@mellanox.com>
>  */
> 
>+#include <linux/device.h>
> #include <net/genetlink.h>
> #define CREATE_TRACE_POINTS
> #include <trace/events/devlink.h>
>@@ -310,6 +311,7 @@ static void devlink_release(struct work_struct *work)
> 
> 	mutex_destroy(&devlink->lock);
> 	lockdep_unregister_key(&devlink->lock_key);
>+	put_device(devlink->dev);
> 	kfree(devlink);
> }
> 
>@@ -425,6 +427,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
> 	if (ret < 0)
> 		goto err_xa_alloc;
> 
>+	get_device(dev);
> 	devlink->dev = dev;

Nit:
	devlink->dev = get_device(dev);


> 	devlink->ops = ops;
> 	xa_init_flags(&devlink->ports, XA_FLAGS_ALLOC);
>-- 
>2.40.1
>
>
Ido Schimmel Oct. 17, 2023, 8:11 a.m. UTC | #2
On Tue, Oct 17, 2023 at 09:56:18AM +0200, Jiri Pirko wrote:
> Tue, Oct 17, 2023 at 09:42:47AM CEST, idosch@nvidia.com wrote:
> >Each devlink instance is associated with a parent device and a pointer
> >to this device is stored in the devlink structure, but devlink does not
> >hold a reference on this device.
> >
> >This is going to be a problem in the next patch where - among other
> >things - devlink will acquire the device lock during netns dismantle,
> >before the reload operation. Since netns dismantle is performed
> >asynchronously and since a reference is not held on the parent device,
> >it will be possible to hit a use-after-free.
> >
> >Prepare for the upcoming change by holding a reference on the parent
> >device.
> >
> 
> Just a note, I'm currently pushing the same patch as a part
> of my patchset:
> https://lore.kernel.org/all/20231013121029.353351-4-jiri@resnulli.us/

Then you probably need patch #1 as well:

https://lore.kernel.org/netdev/20231017074257.3389177-2-idosch@nvidia.com/
Jiri Pirko Oct. 17, 2023, 9:01 a.m. UTC | #3
Tue, Oct 17, 2023 at 10:11:24AM CEST, idosch@nvidia.com wrote:
>On Tue, Oct 17, 2023 at 09:56:18AM +0200, Jiri Pirko wrote:
>> Tue, Oct 17, 2023 at 09:42:47AM CEST, idosch@nvidia.com wrote:
>> >Each devlink instance is associated with a parent device and a pointer
>> >to this device is stored in the devlink structure, but devlink does not
>> >hold a reference on this device.
>> >
>> >This is going to be a problem in the next patch where - among other
>> >things - devlink will acquire the device lock during netns dismantle,
>> >before the reload operation. Since netns dismantle is performed
>> >asynchronously and since a reference is not held on the parent device,
>> >it will be possible to hit a use-after-free.
>> >
>> >Prepare for the upcoming change by holding a reference on the parent
>> >device.
>> >
>> 
>> Just a note, I'm currently pushing the same patch as a part
>> of my patchset:
>> https://lore.kernel.org/all/20231013121029.353351-4-jiri@resnulli.us/
>
>Then you probably need patch #1 as well:
>
>https://lore.kernel.org/netdev/20231017074257.3389177-2-idosch@nvidia.com/

Correct.
diff mbox series

Patch

diff --git a/net/devlink/core.c b/net/devlink/core.c
index bcbbb952569f..5b8b692b8c76 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -4,6 +4,7 @@ 
  * Copyright (c) 2016 Jiri Pirko <jiri@mellanox.com>
  */
 
+#include <linux/device.h>
 #include <net/genetlink.h>
 #define CREATE_TRACE_POINTS
 #include <trace/events/devlink.h>
@@ -310,6 +311,7 @@  static void devlink_release(struct work_struct *work)
 
 	mutex_destroy(&devlink->lock);
 	lockdep_unregister_key(&devlink->lock_key);
+	put_device(devlink->dev);
 	kfree(devlink);
 }
 
@@ -425,6 +427,7 @@  struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
 	if (ret < 0)
 		goto err_xa_alloc;
 
+	get_device(dev);
 	devlink->dev = dev;
 	devlink->ops = ops;
 	xa_init_flags(&devlink->ports, XA_FLAGS_ALLOC);