Re: [qemu-s390x] [PATCH v1 1/1] s390x: fix storage attributes migration for non-small guests

[Cornelia Huck]

On Thu, 18 Jan 2018 18:33:51 +0100
Claudio Imbrenda <address@hidden> wrote:

> On Thu, 18 Jan 2018 18:25:47 +0100
> Cornelia Huck <address@hidden> wrote:
> 
> ...
> [snip]
> 
> > > > > > > diff --git a/hw/s390x/s390-stattrib-kvm.c
> > > > > > > b/hw/s390x/s390-stattrib-kvm.c index 41770a7..480551c 100644
> > > > > > > --- a/hw/s390x/s390-stattrib-kvm.c
> > > > > > > +++ b/hw/s390x/s390-stattrib-kvm.c
> > > > > > > @@ -116,7 +116,7 @@ static void
> > > > > > > kvm_s390_stattrib_synchronize(S390StAttribState *sa) for
> > > > > > > (cx = 0; cx + len <= max; cx += len) { clog.start_gfn = cx;
> > > > > > >              clog.count = len;
> > > > > > > -            clog.values = (uint64_t)(sas->incoming_buffer
> > > > > > > + cx
> > > > > > > * len);            
> > > > > > 
> > > > > > Hm, doesn't that even imply that you reference an area beyond
> > > > > > the buffer, as the <= max check does not catch this?          
> > > > > 
> > > > > what do you mean?
> > > > > 
> > > > > cx + len <= max catches the cases where you would write beyond
> > > > > the end of the buffer. if cx + len == max then we are filling
> > > > > the buffer to the last byte. and we will get out at the next
> > > > > iteration.        
> > > > 
> > > > Yes, but the problem is that your offset is too long, isn't it?
> > > > (Where cx + len <= max, but you use an offset of cx * len which
> > > > may be > max.)      
> > > 
> > > which is exactly why I'm removing that line. look at the very
> > > beginning of the line, there is a -
> > > 
> > > the replacement line (the one that starts with a +) has only cx    
> > 
> > Err, yes :) I simply wanted to comment that this looks worse than "not
> > migrated completely".  
> 
> yeah, that's true :) but the offset ended up big enough to always
> get -EFAULT from the kernel and get ignored by qemu afterwards, which
> then resulted in not all values being migrated.

So the moral is: If you're wrong, be really wrong? :)

> 
> should I change the description to explain the issue in more detail?

Just mentioning something like out-of-bounds due to wrong offset or so
would be good.