On Wed, 6 Dec 2017, Maciej W. Rozycki wrote:
> > > 2. Actually assert what we rely on having been enforced by generic code,
> > > i.e.:
> > >
> > > BUG_ON(*count % sizeof(elf_fpreg_t));
> > > for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > > err = user_regset_copyin(pos, count, kbuf, ubuf,
> > > &fpr_val, i * sizeof(elf_fpreg_t),
> > > (i + 1) * sizeof(elf_fpreg_t));
> > > if (err)
> > > return err;
> > > set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > > }
> > >
> > > so that a discrepancy between generic code and the arch handler is
> > > caught should it happen.
> >
> > The important property is that *count is at least sufficient to fill
> > fpr_val, so that a zero return user_regset_copyin() means fpr_val has
> > been fully initialised with user data.
> >
> > So while your check is not wrong
> >
> > (since *count > 0 && *count % sizeof(elf_fpreg_t) == 0 implies
> > *count >= sizeof(elf_fpreg_t))
> >
> > I don't see how this is an improvement on the original check.
> >
> >
> > Either way, maybe adding a comment to explain the purpose of the check
> > would be a good idea.
>
> I agree a comment is worth having here given the complex dependency.
>
> Therefore, taking what has been observed in the course of this discussion
> into account and unless either you or someone else has further input I am
> going to withdraw this patch from the series as recorded in patchwork and
> submit another one separately that only adds a comment quoting the
> observations made. Obviously it won't be a backport candidate, but 5/5
> does not depend on this change, so I believe there is no need to
> regenerate and repost the remaining changes from this series.
I take it back. After thinking about it some more in the course of
adding the commentary, I realised that with the addition of 2/5 we'll have
a logical inconsistency with the treatment of `count' in that for someone
reading this code without also looking at `ptrace_regset' it will appear
that in the case of a partial register write we call `user_regset_copyin'
to store FCSR having not completely exhausted `count' with FGR accesses.
Which means that for initial `count' being say 12 we'd put the first 8
bytes into $f0 and the following 4 bytes into FCSR rather than into $f1,
which I would find confusing if reading this code the first time. This of
course doesn't matter with our code as it is now, because it does not
access FCSR at all, however this gets changed with 2/5.
So I think that while such a change will have no effect at run time,
because we guarantee that `count % sizeof(elf_fpreg_t) == 0' elsewhere, I
think that using `count > 0' rather than `count >= sizeof(elf_fpreg_t)'
will make code more consistent. If you or anyone else feels uneasy about
`fpr_val' appearing potentially partially uninitialised (even though it
cannot trigger), then I can offer a minimal change for that, which will
preset the variable to 0.
Also to keep things consistent, this patch will have to be reordered
ahead the 2/5 FCSR fix. I will therefore submit v2 of the whole series,
with patches shuffled appropriately, BUG_ON added as previously discussed
as well as an explanatory comment.
Maciej