powerpc/kvm: Handle the boundary condition correctly

Submitted by Aneesh Kumar K.V on Aug. 22, 2013, 11:37 a.m.

Details

Message ID 1377171479-25738-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com
State Not Applicable
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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;