Patchwork powerpc/kvm: Handle the boundary condition correctly

login
register
mail settings
Submitter Aneesh Kumar K.V
Date Aug. 22, 2013, 11:37 a.m.
Message ID <1377171479-25738-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/269027/
State Not Applicable
Headers show

Comments

Aneesh Kumar K.V - Aug. 22, 2013, 11:37 a.m.
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

We should be able to copy upto count bytes

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Alexander Graf - Aug. 22, 2013, 4:48 p.m.
On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

Isn't this you?

> 
> We should be able to copy upto count bytes

Why?


Alex

> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 710d313..0ae6bb6 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -1362,7 +1362,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> 	lbuf = (unsigned long __user *)buf;
> 
> 	nb = 0;
> -	while (nb + sizeof(hdr) + HPTE_SIZE < count) {
> +	while (nb + sizeof(hdr) + HPTE_SIZE <= count) {
> 		/* Initialize header */
> 		hptr = (struct kvm_get_htab_header __user *)buf;
> 		hdr.n_valid = 0;
> @@ -1385,7 +1385,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> 		/* Grab a series of valid entries */
> 		while (i < kvm->arch.hpt_npte &&
> 		       hdr.n_valid < 0xffff &&
> -		       nb + HPTE_SIZE < count &&
> +		       nb + HPTE_SIZE <= count &&
> 		       record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> 			/* valid entry, write it out */
> 			++hdr.n_valid;
> -- 
> 1.8.1.2
>
Aneesh Kumar K.V - Aug. 23, 2013, 3:31 a.m.
Alexander Graf <agraf@suse.de> writes:

> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>
> Isn't this you?

Yes. The patches are generated using git format-patch and sent by
git send-email. That's how it always created patches for me. I am not sure if
there is a config I can change to avoid having From:

>
>> 
>> We should be able to copy upto count bytes
>
> Why?
>

Without this we end up doing

 +    struct kvm_get_htab_buf {
 +        struct kvm_get_htab_header header;
 +        /*
 +         * Older kernel required one extra byte.
 +         */
 +        unsigned long hpte[3];
 +    } hpte_buf;


even though we are only looking for one hpte entry.

http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com

>
> Alex
>
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> index 710d313..0ae6bb6 100644
>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> @@ -1362,7 +1362,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
>> 	lbuf = (unsigned long __user *)buf;
>> 
>> 	nb = 0;
>> -	while (nb + sizeof(hdr) + HPTE_SIZE < count) {
>> +	while (nb + sizeof(hdr) + HPTE_SIZE <= count) {
>> 		/* Initialize header */
>> 		hptr = (struct kvm_get_htab_header __user *)buf;
>> 		hdr.n_valid = 0;
>> @@ -1385,7 +1385,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
>> 		/* Grab a series of valid entries */
>> 		while (i < kvm->arch.hpt_npte &&
>> 		       hdr.n_valid < 0xffff &&
>> -		       nb + HPTE_SIZE < count &&
>> +		       nb + HPTE_SIZE <= count &&
>> 		       record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
>> 			/* valid entry, write it out */
>> 			++hdr.n_valid;
>> -- 
>> 1.8.1.2
>>
Benjamin Herrenschmidt - Aug. 23, 2013, 4:28 a.m.
On Fri, 2013-08-23 at 09:01 +0530, Aneesh Kumar K.V wrote:
> Alexander Graf <agraf@suse.de> writes:
> 
> > On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
> >
> >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> >
> > Isn't this you?
> 
> Yes. The patches are generated using git format-patch and sent by
> git send-email. That's how it always created patches for me. I am not sure if
> there is a config I can change to avoid having From:

Don't bother, that's perfectly fine, and git am will do the right thing.

Cheers,
Ben.

> >
> >> 
> >> We should be able to copy upto count bytes
> >
> > Why?
> >
> 
> Without this we end up doing
> 
>  +    struct kvm_get_htab_buf {
>  +        struct kvm_get_htab_header header;
>  +        /*
>  +         * Older kernel required one extra byte.
>  +         */
>  +        unsigned long hpte[3];
>  +    } hpte_buf;
> 
> 
> even though we are only looking for one hpte entry.
> 
> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
> 
> >
> > Alex
> >
> >> 
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> >> ---
> >> arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> >> index 710d313..0ae6bb6 100644
> >> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> >> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> >> @@ -1362,7 +1362,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> >> 	lbuf = (unsigned long __user *)buf;
> >> 
> >> 	nb = 0;
> >> -	while (nb + sizeof(hdr) + HPTE_SIZE < count) {
> >> +	while (nb + sizeof(hdr) + HPTE_SIZE <= count) {
> >> 		/* Initialize header */
> >> 		hptr = (struct kvm_get_htab_header __user *)buf;
> >> 		hdr.n_valid = 0;
> >> @@ -1385,7 +1385,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> >> 		/* Grab a series of valid entries */
> >> 		while (i < kvm->arch.hpt_npte &&
> >> 		       hdr.n_valid < 0xffff &&
> >> -		       nb + HPTE_SIZE < count &&
> >> +		       nb + HPTE_SIZE <= count &&
> >> 		       record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> >> 			/* valid entry, write it out */
> >> 			++hdr.n_valid;
> >> -- 
> >> 1.8.1.2
> >>
Alexander Graf - Aug. 25, 2013, 6:09 p.m.
On 23.08.2013, at 05:28, Benjamin Herrenschmidt wrote:

> On Fri, 2013-08-23 at 09:01 +0530, Aneesh Kumar K.V wrote:
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>>> 
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> Isn't this you?
>> 
>> Yes. The patches are generated using git format-patch and sent by
>> git send-email. That's how it always created patches for me. I am not sure if
>> there is a config I can change to avoid having From:
> 
> Don't bother, that's perfectly fine, and git am will do the right thing.

It will, but it's an indicator that something in his git config is misconfigured. Usually when git sees "Author == Sender" it will omit the From: line.


Alex
Alexander Graf - Aug. 25, 2013, 6:16 p.m.
On 23.08.2013, at 04:31, Aneesh Kumar K.V wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> Isn't this you?
> 
> Yes. The patches are generated using git format-patch and sent by
> git send-email. That's how it always created patches for me. I am not sure if
> there is a config I can change to avoid having From:
> 
>> 
>>> 
>>> We should be able to copy upto count bytes
>> 
>> Why?
>> 
> 
> Without this we end up doing
> 
> +    struct kvm_get_htab_buf {
> +        struct kvm_get_htab_header header;
> +        /*
> +         * Older kernel required one extra byte.
> +         */
> +        unsigned long hpte[3];
> +    } hpte_buf;
> 
> 
> even though we are only looking for one hpte entry.

Ok, please give me an example with real numbers and why it breaks.


Alex

> 
> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
> 
>> 
>> Alex
>> 
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> index 710d313..0ae6bb6 100644
>>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> @@ -1362,7 +1362,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
>>> 	lbuf = (unsigned long __user *)buf;
>>> 
>>> 	nb = 0;
>>> -	while (nb + sizeof(hdr) + HPTE_SIZE < count) {
>>> +	while (nb + sizeof(hdr) + HPTE_SIZE <= count) {
>>> 		/* Initialize header */
>>> 		hptr = (struct kvm_get_htab_header __user *)buf;
>>> 		hdr.n_valid = 0;
>>> @@ -1385,7 +1385,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
>>> 		/* Grab a series of valid entries */
>>> 		while (i < kvm->arch.hpt_npte &&
>>> 		       hdr.n_valid < 0xffff &&
>>> -		       nb + HPTE_SIZE < count &&
>>> +		       nb + HPTE_SIZE <= count &&
>>> 		       record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
>>> 			/* valid entry, write it out */
>>> 			++hdr.n_valid;
>>> -- 
>>> 1.8.1.2
>>> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V - Aug. 26, 2013, 3:28 a.m.
Alexander Graf <agraf@suse.de> writes:

> On 23.08.2013, at 04:31, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>>> 
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> Isn't this you?
>> 
>> Yes. The patches are generated using git format-patch and sent by
>> git send-email. That's how it always created patches for me. I am not sure if
>> there is a config I can change to avoid having From:
>> 
>>> 
>>>> 
>>>> We should be able to copy upto count bytes
>>> 
>>> Why?
>>> 
>> 
>> Without this we end up doing
>> 
>> +    struct kvm_get_htab_buf {
>> +        struct kvm_get_htab_header header;
>> +        /*
>> +         * Older kernel required one extra byte.
>> +         */
>> +        unsigned long hpte[3];
>> +    } hpte_buf;
>> 
>> 
>> even though we are only looking for one hpte entry.
>
> Ok, please give me an example with real numbers and why it breaks.
>
>> 
>> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>> 

Didn't quiet get what you are looking for. As explained before, we now
need to pass an array with array size 3 even though we know we need to
read only 2 entries because kernel doesn't loop correctly.

-aneesh
Alexander Graf - Aug. 26, 2013, 11:10 a.m.
On 26.08.2013, at 05:28, Aneesh Kumar K.V wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 23.08.2013, at 04:31, Aneesh Kumar K.V wrote:
>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>>>> 
>>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>> 
>>>> Isn't this you?
>>> 
>>> Yes. The patches are generated using git format-patch and sent by
>>> git send-email. That's how it always created patches for me. I am not sure if
>>> there is a config I can change to avoid having From:
>>> 
>>>> 
>>>>> 
>>>>> We should be able to copy upto count bytes
>>>> 
>>>> Why?
>>>> 
>>> 
>>> Without this we end up doing
>>> 
>>> +    struct kvm_get_htab_buf {
>>> +        struct kvm_get_htab_header header;
>>> +        /*
>>> +         * Older kernel required one extra byte.
>>> +         */
>>> +        unsigned long hpte[3];
>>> +    } hpte_buf;
>>> 
>>> 
>>> even though we are only looking for one hpte entry.
>> 
>> Ok, please give me an example with real numbers and why it breaks.
>> 
>>> 
>>> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>>> 
> 
> Didn't quiet get what you are looking for. As explained before, we now
> need to pass an array with array size 3 even though we know we need to
> read only 2 entries because kernel doesn't loop correctly.

But we need to do that regardless, because newer QEMU needs to be able to run on older kernels, no?


Alex
Aneesh Kumar K.V - Aug. 26, 2013, 12:14 p.m.
Alexander Graf <agraf@suse.de> writes:

> On 26.08.2013, at 05:28, Aneesh Kumar K.V wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 23.08.2013, at 04:31, Aneesh Kumar K.V wrote:
>>> 
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> On 22.08.2013, at 12:37, Aneesh Kumar K.V wrote:
>>>>> 
>>>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>> 
>>>>> Isn't this you?
>>>> 
>>>> Yes. The patches are generated using git format-patch and sent by
>>>> git send-email. That's how it always created patches for me. I am not sure if
>>>> there is a config I can change to avoid having From:
>>>> 
>>>>> 
>>>>>> 
>>>>>> We should be able to copy upto count bytes
>>>>> 
>>>>> Why?
>>>>> 
>>>> 
>>>> Without this we end up doing
>>>> 
>>>> +    struct kvm_get_htab_buf {
>>>> +        struct kvm_get_htab_header header;
>>>> +        /*
>>>> +         * Older kernel required one extra byte.
>>>> +         */
>>>> +        unsigned long hpte[3];
>>>> +    } hpte_buf;
>>>> 
>>>> 
>>>> even though we are only looking for one hpte entry.
>>> 
>>> Ok, please give me an example with real numbers and why it breaks.
>>> 
>>>> 
>>>> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>>>> 
>> 
>> Didn't quiet get what you are looking for. As explained before, we now
>> need to pass an array with array size 3 even though we know we need to
>> read only 2 entries because kernel doesn't loop correctly.
>
> But we need to do that regardless, because newer QEMU needs to be able to run on older kernels, no?
>

yes. So use space will have to pass an array of size 3. But that should
not prevent us from fixing this right ?

-aneesh
Aneesh Kumar K.V - Sept. 25, 2013, 3:42 p.m.
Hi Alex,

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>>>> Ok, please give me an example with real numbers and why it breaks.
>>>> 
>>>>> 
>>>>> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>>>>> 
>>> 
>>> Didn't quiet get what you are looking for. As explained before, we now
>>> need to pass an array with array size 3 even though we know we need to
>>> read only 2 entries because kernel doesn't loop correctly.
>>
>> But we need to do that regardless, because newer QEMU needs to be able to run on older kernels, no?
>>
>
> yes. So use space will have to pass an array of size 3. But that should
> not prevent us from fixing this right ?
>

Do we still want this patch or should I drop this ?

-aneesh
Alexander Graf - Sept. 25, 2013, 3:51 p.m.
On 25.09.2013, at 17:42, Aneesh Kumar K.V wrote:

> 
> Hi Alex,
> 
> "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
>>>>> Ok, please give me an example with real numbers and why it breaks.
>>>>> 
>>>>>> 
>>>>>> http://mid.gmane.org/1376995766-16526-4-git-send-email-aneesh.kumar@linux.vnet.ibm.com
>>>>>> 
>>>> 
>>>> Didn't quiet get what you are looking for. As explained before, we now
>>>> need to pass an array with array size 3 even though we know we need to
>>>> read only 2 entries because kernel doesn't loop correctly.
>>> 
>>> But we need to do that regardless, because newer QEMU needs to be able to run on older kernels, no?
>>> 
>> 
>> yes. So use space will have to pass an array of size 3. But that should
>> not prevent us from fixing this right ?
>> 
> 
> Do we still want this patch or should I drop this ?

If we really want it we need to be able to tell user space that we support this feature through a CAP, so that it can act accordingly. Given the additional complexity of this and the fact that we do want to support older KVM versions regardless, I don't think it's worth the hassle.


Alex

Patch

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 710d313..0ae6bb6 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -1362,7 +1362,7 @@  static ssize_t kvm_htab_read(struct file *file, char __user *buf,
 	lbuf = (unsigned long __user *)buf;
 
 	nb = 0;
-	while (nb + sizeof(hdr) + HPTE_SIZE < count) {
+	while (nb + sizeof(hdr) + HPTE_SIZE <= count) {
 		/* Initialize header */
 		hptr = (struct kvm_get_htab_header __user *)buf;
 		hdr.n_valid = 0;
@@ -1385,7 +1385,7 @@  static ssize_t kvm_htab_read(struct file *file, char __user *buf,
 		/* Grab a series of valid entries */
 		while (i < kvm->arch.hpt_npte &&
 		       hdr.n_valid < 0xffff &&
-		       nb + HPTE_SIZE < count &&
+		       nb + HPTE_SIZE <= count &&
 		       record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
 			/* valid entry, write it out */
 			++hdr.n_valid;