Some error handling issues I noticed while looking at the code.
Only compile-tested.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- Changes in v2: - Fix typo in commit message: vmclock_ptp_register() - Retarget against net tree - Include patch from David. It's at the start of the series so that all bugfixes are at the beginning and the logical ordering of my patches is not disrupted. - Pick up tags from LKML - Link to v1: https://lore.kernel.org/r/20250206-vmclock-probe-v1-0-17a3ea07be34@linutroni...
--- David Woodhouse (1): ptp: vmclock: Add .owner to vmclock_miscdev_fops
Thomas Weißschuh (4): ptp: vmclock: Set driver data before its usage ptp: vmclock: Don't unregister misc device if it was not registered ptp: vmclock: Clean up miscdev and ptp clock through devres ptp: vmclock: Remove goto-based cleanup logic
drivers/ptp/ptp_vmclock.c | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) --- base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b change-id: 20250206-vmclock-probe-57cbcb770925
Best regards,
From: David Woodhouse dwmw@amazon.co.uk
Without the .owner field, the module can be unloaded while /dev/vmclock0 is open, leading to an oops.
Fixes: 205032724226 ("ptp: Add support for the AMZNC10C 'vmclock' device") Cc: stable@vger.kernel.org Signed-off-by: David Woodhouse dwmw@amazon.co.uk Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de --- drivers/ptp/ptp_vmclock.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c index 0a2cfc8ad3c5408a87fd8fedeff274ab895de3dd..dbc73e5382935dcbc799ea608b87f5ff51044ebc 100644 --- a/drivers/ptp/ptp_vmclock.c +++ b/drivers/ptp/ptp_vmclock.c @@ -414,6 +414,7 @@ static ssize_t vmclock_miscdev_read(struct file *fp, char __user *buf, }
static const struct file_operations vmclock_miscdev_fops = { + .owner = THIS_MODULE, .mmap = vmclock_miscdev_mmap, .read = vmclock_miscdev_read, };
If vmclock_ptp_register() fails during probing, vmclock_remove() is called to clean up the ptp clock and misc device. It uses dev_get_drvdata() to access the vmclock state. However the driver data is not yet set at this point.
Assign the driver data earlier.
Fixes: 205032724226 ("ptp: Add support for the AMZNC10C 'vmclock' device") Cc: stable@vger.kernel.org Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de Reviewed-by: Mateusz Polchlopek mateusz.polchlopek@intel.com Acked-by: Richard Cochran richardcochran@gmail.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- drivers/ptp/ptp_vmclock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c index dbc73e5382935dcbc799ea608b87f5ff51044ebc..1ba30a2da570fb4d1ec9db72820bf1781dfa9655 100644 --- a/drivers/ptp/ptp_vmclock.c +++ b/drivers/ptp/ptp_vmclock.c @@ -525,6 +525,8 @@ static int vmclock_probe(struct platform_device *pdev) goto out; }
+ dev_set_drvdata(dev, st); + if (le32_to_cpu(st->clk->magic) != VMCLOCK_MAGIC || le32_to_cpu(st->clk->size) > resource_size(&st->res) || le16_to_cpu(st->clk->version) != 1) { @@ -588,8 +590,6 @@ static int vmclock_probe(struct platform_device *pdev) (st->miscdev.minor && st->ptp_clock) ? ", " : "", st->ptp_clock ? "PTP" : "");
- dev_set_drvdata(dev, st); - out: return ret; }
vmclock_remove() tries to detect the successful registration of the misc device based on the value of its minor value. However that check is incorrect if the misc device registration was not attempted in the first place.
Always initialize the minor number, so the check works properly.
Fixes: 205032724226 ("ptp: Add support for the AMZNC10C 'vmclock' device") Cc: stable@vger.kernel.org Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de Acked-by: Richard Cochran richardcochran@gmail.com Reviewed-by: David Woodhouse dwmw@amazon.co.uk --- drivers/ptp/ptp_vmclock.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c index 1ba30a2da570fb4d1ec9db72820bf1781dfa9655..9b8bd626a397313433908fcc838edf8ffc3ecc98 100644 --- a/drivers/ptp/ptp_vmclock.c +++ b/drivers/ptp/ptp_vmclock.c @@ -550,6 +550,8 @@ static int vmclock_probe(struct platform_device *pdev) goto out; }
+ st->miscdev.minor = MISC_DYNAMIC_MINOR; + /* * If the structure is big enough, it can be mapped to userspace. * Theoretically a guest OS even using larger pages could still @@ -557,7 +559,6 @@ static int vmclock_probe(struct platform_device *pdev) * cross that bridge if/when we come to it. */ if (le32_to_cpu(st->clk->size) >= PAGE_SIZE) { - st->miscdev.minor = MISC_DYNAMIC_MINOR; st->miscdev.fops = &vmclock_miscdev_fops; st->miscdev.name = st->name;
Hello:
This series was applied to netdev/net.git (main) by Paolo Abeni pabeni@redhat.com:
On Fri, 07 Feb 2025 10:39:01 +0100 you wrote:
Some error handling issues I noticed while looking at the code.
Only compile-tested.
Signed-off-by: Thomas Weißschuh thomas.weissschuh@linutronix.de
Changes in v2:
- Fix typo in commit message: vmclock_ptp_register()
- Retarget against net tree
- Include patch from David. It's at the start of the series so that all bugfixes are at the beginning and the logical ordering of my patches is not disrupted.
- Pick up tags from LKML
- Link to v1: https://lore.kernel.org/r/20250206-vmclock-probe-v1-0-17a3ea07be34@linutroni...
[...]
Here is the summary with links: - [net,v2,1/5] ptp: vmclock: Add .owner to vmclock_miscdev_fops https://git.kernel.org/netdev/net/c/7b07b040257c - [net,v2,2/5] ptp: vmclock: Set driver data before its usage https://git.kernel.org/netdev/net/c/f7d07cd4f77d - [net,v2,3/5] ptp: vmclock: Don't unregister misc device if it was not registered https://git.kernel.org/netdev/net/c/39e926c3a21b - [net,v2,4/5] ptp: vmclock: Clean up miscdev and ptp clock through devres https://git.kernel.org/netdev/net/c/9a884c3800b2 - [net,v2,5/5] ptp: vmclock: Remove goto-based cleanup logic https://git.kernel.org/netdev/net/c/b4c1fde5ced9
You are awesome, thank you!
linux-stable-mirror@lists.linaro.org
-
patchwork-bot+netdevbpf＠kernel.org -
Thomas Weißschuh