netdev - Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210701145935.GB22819@hoboy.vegasvil.org>
Date:   Thu, 1 Jul 2021 07:59:35 -0700
From:   Richard Cochran <richardcochran@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     netdev@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH] ptp: Add PTP_CLOCK_EXTTSUSR internal ptp_event

On Tue, Jun 29, 2021 at 08:50:31PM -0700, Jonathan Lemon wrote:

> Since these events are channel specific, I don't see why
> this is problematic.

The problem is that the semantics of ptp_clock_event::data are not
defined...

> The code blocks in question from my
> upcoming patch (dependent on this) is:
> 
>     static irqreturn_t
>     ptp_ocp_phase_irq(int irq, void *priv)
>     {
>             struct ptp_ocp_ext_src *ext = priv;
>             struct ocp_phase_reg __iomem *reg = ext->mem;
>             struct ptp_clock_event ev;
>             u32 phase_error;
> 
>             phase_error = ioread32(&reg->phase_error);
> 
>             ev.type = PTP_CLOCK_EXTTSUSR;
>             ev.index = ext->info->index;
>             ev.data = phase_error;
>             pps_get_ts(&ev.pps_times);

Here the time stamp is the system time, and the .data field is the
mysterious "phase_error", but ...
 
>             ptp_clock_event(ext->bp->ptp, &ev);
> 
>             iowrite32(0, &reg->intr);
> 
>             return IRQ_HANDLED;
>     }
> 
> and
> 
>     static irqreturn_t
>     ptp_ocp_ts_irq(int irq, void *priv)
>     {
>             struct ptp_ocp_ext_src *ext = priv;
>             struct ts_reg __iomem *reg = ext->mem;
>             struct ptp_clock_event ev;
> 
>             ev.type = PTP_CLOCK_EXTTSUSR;
>             ev.index = ext->info->index;
>             ev.pps_times.ts_real.tv_sec = ioread32(&reg->time_sec);
>             ev.pps_times.ts_real.tv_nsec = ioread32(&reg->time_ns);
>             ev.data = ioread32(&reg->ts_count);

here the time stamp comes from the PHC device, apparently, and the
.data field is a counter.

>             ptp_clock_event(ext->bp->ptp, &ev);
> 
>             iowrite32(1, &reg->intr);       /* write 1 to ack */
> 
>             return IRQ_HANDLED;
>     }
> 
> 
> I'm not seeing why this is controversial.

Simply put, it makes no sense to provide a new PTP_CLOCK_EXTTSUSR that
has multiple, random meanings.  There has got to be a better way.

Thanks,
Richard