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: Thu, 1 Dec 2022 03:04:57 +0200 From: Vladimir Oltean <olteanv@...il.com> To: Arun Ramadoss <arun.ramadoss@...rochip.com> Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com, andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, linux@...linux.org.uk, Tristram.Ha@...rochip.com, richardcochran@...il.com, ceggers@...i.de Subject: Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock On Mon, Nov 28, 2022 at 04:02:19PM +0530, Arun Ramadoss wrote: > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c > index 184aa57a8489..415522ef4ce9 100644 > --- a/drivers/net/dsa/microchip/ksz_ptp.c > +++ b/drivers/net/dsa/microchip/ksz_ptp.c > @@ -200,6 +209,12 @@ static int ksz_ptp_settime(struct ptp_clock_info *ptp, > goto error_return; > > ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME, PTP_LOAD_TIME); > + if (ret) > + goto error_return; > + > + spin_lock_bh(&ptp_data->clock_lock); Why disable bottom halves? Where is the bottom half that this races with? > + ptp_data->clock_time = *ts; > + spin_unlock_bh(&ptp_data->clock_lock); > > error_return: > mutex_unlock(&ptp_data->lock); > @@ -254,6 +269,7 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) > { > struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp); > struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data); > + struct timespec64 delta64 = ns_to_timespec64(delta); > s32 sec, nsec; > u16 data16; > int ret; > @@ -286,15 +302,51 @@ static int ksz_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta) > data16 |= PTP_STEP_DIR; > > ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16); > + if (ret) > + goto error_return; > + > + spin_lock_bh(&ptp_data->clock_lock); > + ptp_data->clock_time = timespec64_add(ptp_data->clock_time, delta64); > + spin_unlock_bh(&ptp_data->clock_lock); > > error_return: > mutex_unlock(&ptp_data->lock); > return ret; > } > > +/* Function is pointer to the do_aux_work in the ptp_clock capability */ > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp) > +{ > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp); > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data); > + struct timespec64 ts; > + > + mutex_lock(&ptp_data->lock); > + _ksz_ptp_gettime(dev, &ts); > + mutex_unlock(&ptp_data->lock); Why don't you call ksz_ptp_gettime(ptp, &ts) directly? > + > + spin_lock_bh(&ptp_data->clock_lock); > + ptp_data->clock_time = ts; > + spin_unlock_bh(&ptp_data->clock_lock); > + > + return HZ; /* reschedule in 1 second */ > +} > + > static int ksz_ptp_start_clock(struct ksz_device *dev) > { > - return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE); > + struct ksz_ptp_data *ptp_data = &dev->ptp_data; > + int ret; > + > + ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE, PTP_CLK_ENABLE); > + if (ret) > + return ret; > + > + spin_lock_bh(&ptp_data->clock_lock); > + ptp_data->clock_time.tv_sec = 0; > + ptp_data->clock_time.tv_nsec = 0; > + spin_unlock_bh(&ptp_data->clock_lock); Does ksz_ptp_start_clock() race with anything? The PTP clock has not even been registered by the time this has been called. This is literally an example of the "spin_lock_init(); spin_lock();" antipattern. > + > + return 0; > }
Powered by blists - more mailing lists