linux-kernel - RE: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions

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]

Date:   Thu, 12 Oct 2017 15:54:48 +0000
From:   Aleksandar Markovic <Aleksandar.Markovic@...tec.com>
To:     James Hogan <james.hogan@...s.com>,
        Aleksandar Markovic <Aleksandar.Markovic@...rk.com>
CC:     Miodrag Dinic <Miodrag.Dinic@...tec.com>,
        Paul Burton <Paul.Burton@...tec.com>,
        Petar Jovanovic <Petar.Jovanovic@...tec.com>,
        Raghu Gandham <Raghu.Gandham@...tec.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        "linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
        Douglas Leung <Douglas.Leung@...tec.com>,
        "Goran Ferenc" <Goran.Ferenc@...tec.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Maciej Rozycki <Maciej.Rozycki@...tec.com>,
        Manuel Lauss <manuel.lauss@...il.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
Subject: RE: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats
 for certain instructions

> Right, but its not going to even get to copcsr until this patch, so the
> SIGFPE handling is I think fixed by this patch, i.e. it isn't just about
> the stats.

On more detailed code inspection, you are right.

> This patch fixes something, I think it should
> a) be clear in the commit message what is fixed
> b) be tagged for stable (though that can always be done
> retrospectively)

If you agree, I am going to submit v2 of the series, that would fully
address these concerns.

Additionally, it seems to me that a new round of testing that tests
involved code paths under various scenarios would be appropriate
and I am going to do that.

> Note: thats the one in fpux_emu(), not fpu_emu() which this patch
> modifies.

Yes, my bad, wanting to respond as quickly as possible, I inserted
the segment from fpux_emu(), not fpu_emu() as I should have.

By the way, and not related to this patch, I see only 4 (out of 5)
exceptions are handled in fpux_emu() case (division-by-zero is not
handled), I presume this is fine (probably division-by-zero not
needed), isn't it?

I truly appreciate your analysis and help.

Aleksandar

________________________________________
From: James Hogan [james.hogan@...s.com]
Sent: Thursday, October 12, 2017 7:44 AM
To: Aleksandar Markovic
Cc: Miodrag Dinic; Paul Burton; Petar Jovanovic; Raghu Gandham; Ralf Baechle; Aleksandar Markovic; linux-mips@...ux-mips.org; Douglas Leung; Goran Ferenc; linux-kernel@...r.kernel.org; Maciej Rozycki; Manuel Lauss; Masahiro Yamada
Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions

On Thu, Oct 12, 2017 at 03:57:50PM +0200, Aleksandar Markovic wrote:
>
> > Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions
> > Date: Thursday, October 12, 2017 12:17 CEST
> > From: James Hogan <james.hogan@...s.com>>@badag02.ba.imgtec.org>
> > > ...
> > > if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E)
> > > ...
> >
> > But just before that condition it does:
> >
> > ctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;
> > I.e. it clears the X bits used in the condition, and overrides them,
> > based on rcsr, which is initialised to 0 and is only set after the
> > copcsr label and in a couple of other cases I don't think we'd be
> > hitting for MADDF.
> >
>
> The code is odd and deceiving here. Let's see the whole "copcsr label"
> code segment: copcsr:if (ieee754_cxtest(IEEE754_INEXACT)) {  MIPS_FPU_EMU_INC_STATS(ieee754_inexact);  rcsr |= FPU_CSR_INE_X | FPU_CSR_INE_S;}if (ieee754_cxtest(IEEE754_UNDERFLOW)) {  MIPS_FPU_EMU_INC_STATS(ieee754_underflow);  rcsr |= FPU_CSR_UDF_X | FPU_CSR_UDF_S;}if (ieee754_cxtest(IEEE754_OVERFLOW)) {  MIPS_FPU_EMU_INC_STATS(ieee754_overflow);  rcsr |= FPU_CSR_OVF_X | FPU_CSR_OVF_S;}  if (ieee754_cxtest(IEEE754_INVALID_OPERATION)) {  MIPS_FPU_EMU_INC_STATS(ieee754_invalidop);  rcsr |= FPU_CSR_INV_X | FPU_CSR_INV_S;} ctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E) {  /*printk ("SIGFPE: FPU csr = %08x\n",  ctx->fcr31); */  return SIGFPE;}

Note: thats the one in fpux_emu(), not fpu_emu() which this patch
modifies. In fpu_emu() the copying of bits from rcsr to fcr32 and the
SIGFPE checking takes place outside of the switch, after other stuff can
modify rcsr.

>
>
> Value of rcsr will be dictated by series of invocations to ieee754_cxtest(),
> which, in fact, means that exception bits will be copied from fcr31 to rcsr.
>
> Then, fcr31 exception bits are cleared and set to the values they had just
> before clearing.
>
> Obviously, this will not do anything in our scenarios.
>
> However, the patch is about correct setting of debugfs stats, and this code
> segment correctly does this.



>
> May I suggest that we accept my patch as is, and if anybody for any reason
> wants to deal further with related code, this should be done in a separate
> fix/patch?

This patch fixes something, I think it should
a) be clear in the commit message what is fixed
b) be tagged for stable (though that can always be done
retrospectively).

Cheers
James