| Message ID | 20250123-ptp-enable-v1-1-b015834d3a47@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESEND,net] ptp: Ensure info->enable callback is always set | expand |
On Thu, Jan 23, 2025 at 08:22:40AM +0100, Thomas Weißschuh wrote: > The ioctl and sysfs handlers unconditionally call the ->enable callback. > Not all drivers implement that callback, leading to NULL dereferences. > Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c. > > Instead use a dummy callback if no better was specified by the driver. > > Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.") > Cc: stable@vger.kernel.org > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/ptp/ptp_clock.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts) > return info->gettime64(info, ts); > } > > +static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on) > +{ > + return -EOPNOTSUPP; > +} > + > static void ptp_aux_kworker(struct kthread_work *work) > { > struct ptp_clock *ptp = container_of(work, struct ptp_clock, > @@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, > ptp->info->getcrosscycles = ptp->info->getcrosststamp; > } > > + if (!ptp->info->enable) > + ptp->info->enable = ptp_enable; > + > if (ptp->info->do_aux_work) { > kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker); > ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index); > > --- > base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82 > change-id: 20250122-ptp-enable-831339c62428 > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> What about other ops, did you check it too? Looks like it isn't needed, but it sometimes hard to follow. Thanks
On 2025-01-23 10:38:37+0100, Michal Swiatkowski wrote: > On Thu, Jan 23, 2025 at 08:22:40AM +0100, Thomas Weißschuh wrote: > > The ioctl and sysfs handlers unconditionally call the ->enable callback. > > Not all drivers implement that callback, leading to NULL dereferences. > > Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c. > > > > Instead use a dummy callback if no better was specified by the driver. > > > > Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.") > > Cc: stable@vger.kernel.org > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > > --- > > drivers/ptp/ptp_clock.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > > index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644 > > --- a/drivers/ptp/ptp_clock.c > > +++ b/drivers/ptp/ptp_clock.c > > @@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts) > > return info->gettime64(info, ts); > > } > > > > +static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on) > > +{ > > + return -EOPNOTSUPP; > > +} > > + > > static void ptp_aux_kworker(struct kthread_work *work) > > { > > struct ptp_clock *ptp = container_of(work, struct ptp_clock, > > @@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, > > ptp->info->getcrosscycles = ptp->info->getcrosststamp; > > } > > > > + if (!ptp->info->enable) > > + ptp->info->enable = ptp_enable; > > + > > if (ptp->info->do_aux_work) { > > kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker); > > ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index); > > > > --- > > base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82 > > change-id: 20250122-ptp-enable-831339c62428 > > > > Best regards, > > -- > > Thomas Weißschuh <linux@weissschuh.net> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > What about other ops, did you check it too? Looks like it isn't needed, > but it sometimes hard to follow. I couldn't find any missing, but I'm not familiar with the subsystem and didn't check too hard. Note: A follow-up fix would be to actually guard the users of ->enable and error out. For sysfs the attributes could be hidden completely. That would be a nicer user interface but more code change which are not so easily backportable. Thomas
On Thu, Jan 23, 2025 at 02:19:46PM +0100, Thomas Weißschuh wrote: > On 2025-01-23 10:38:37+0100, Michal Swiatkowski wrote: > > What about other ops, did you check it too? Looks like it isn't needed, > > but it sometimes hard to follow. > > I couldn't find any missing, but I'm not familiar with the subsystem and > didn't check too hard. Initially all of the callbacks were required, but that requirement became relaxed over time with getcycles64(). Now that we have more and more drivers, it wouldn't hurt to let ptp_clock_register() check that the needed callbacks are valid. > Note: > > A follow-up fix would be to actually guard the users of ->enable and > error out. Yes, I would place checks at the call sites, within ptp_ioctl(). Thanks, Richard
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index b932425ddc6a3789504164a69d1b8eba47da462c..35a5994bf64f6373c08269d63aaeac3f4ab31ff0 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -217,6 +217,11 @@ static int ptp_getcycles64(struct ptp_clock_info *info, struct timespec64 *ts) return info->gettime64(info, ts); } +static int ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *request, int on) +{ + return -EOPNOTSUPP; +} + static void ptp_aux_kworker(struct kthread_work *work) { struct ptp_clock *ptp = container_of(work, struct ptp_clock, @@ -294,6 +299,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, ptp->info->getcrosscycles = ptp->info->getcrosststamp; } + if (!ptp->info->enable) + ptp->info->enable = ptp_enable; + if (ptp->info->do_aux_work) { kthread_init_delayed_work(&ptp->aux_work, ptp_aux_kworker); ptp->kworker = kthread_run_worker(0, "ptp%d", ptp->index);
The ioctl and sysfs handlers unconditionally call the ->enable callback. Not all drivers implement that callback, leading to NULL dereferences. Example of affected drivers: ptp_s390.c, ptp_vclock.c and ptp_mock.c. Instead use a dummy callback if no better was specified by the driver. Fixes: d94ba80ebbea ("ptp: Added a brand new class driver for ptp clocks.") Cc: stable@vger.kernel.org Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/ptp/ptp_clock.c | 8 ++++++++ 1 file changed, 8 insertions(+) --- base-commit: c4b9570cfb63501638db720f3bee9f6dfd044b82 change-id: 20250122-ptp-enable-831339c62428 Best regards,