We check that the struct vm_area_struct pointer vma is NULL and then
dereference it a few lines below. The intent must have been to make sure
that vma is not NULL and then to check the value from cpu_context() for
the condition to be true.
Signed-off-by: Emil Goode <[email protected]>
---
v2: Updated the commit message with a better explanation.
arch/mips/mm/tlb-r3k.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
index d657493..6546758 100644
--- a/arch/mips/mm/tlb-r3k.c
+++ b/arch/mips/mm/tlb-r3k.c
@@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
{
int cpu = smp_processor_id();
- if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
+ if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
int oldpid, newpid, idx;
--
1.7.10.4
On Sat, Jul 5, 2014 at 8:26 PM, Emil Goode <[email protected]> wrote:
> We check that the struct vm_area_struct pointer vma is NULL and then
> dereference it a few lines below. The intent must have been to make sure
> that vma is not NULL and then to check the value from cpu_context() for
> the condition to be true.
>
> Signed-off-by: Emil Goode <[email protected]>
> ---
>
> v2: Updated the commit message with a better explanation.
>
> arch/mips/mm/tlb-r3k.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> index d657493..6546758 100644
> --- a/arch/mips/mm/tlb-r3k.c
> +++ b/arch/mips/mm/tlb-r3k.c
> @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> {
> int cpu = smp_processor_id();
>
> - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
Sorry for replying "too late", but grepping through the kernel code I
fail to find any caller that does not dereference vma before calling
(local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
be NULL, so I would say it is safe to assume vma is never NULL, and
the NULL check can be removed completely.
Also it looks like this "bug" was there since at least 2.6.12, and
never seem to have bitten anyone.
Jonas
Hello,
On Sat, Jul 05, 2014 at 09:10:44PM +0200, Jonas Gorski wrote:
> On Sat, Jul 5, 2014 at 8:26 PM, Emil Goode <[email protected]> wrote:
> > We check that the struct vm_area_struct pointer vma is NULL and then
> > dereference it a few lines below. The intent must have been to make sure
> > that vma is not NULL and then to check the value from cpu_context() for
> > the condition to be true.
> >
> > Signed-off-by: Emil Goode <[email protected]>
> > ---
> >
> > v2: Updated the commit message with a better explanation.
> >
> > arch/mips/mm/tlb-r3k.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > index d657493..6546758 100644
> > --- a/arch/mips/mm/tlb-r3k.c
> > +++ b/arch/mips/mm/tlb-r3k.c
> > @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> > {
> > int cpu = smp_processor_id();
> >
> > - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> > + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
>
> Sorry for replying "too late", but grepping through the kernel code I
> fail to find any caller that does not dereference vma before calling
> (local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
> be NULL, so I would say it is safe to assume vma is never NULL, and
> the NULL check can be removed completely.
>
> Also it looks like this "bug" was there since at least 2.6.12, and
> never seem to have bitten anyone.
Yes, the bug pre-dates GIT history and I agree that it is most unlikely
that it ever caused a problem. I will send a new patch that removes the
NULL check of vma.
Best regards,
Emil Goode
On Sun, 6 Jul 2014, Emil Goode wrote:
> > > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > > index d657493..6546758 100644
> > > --- a/arch/mips/mm/tlb-r3k.c
> > > +++ b/arch/mips/mm/tlb-r3k.c
> > > @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> > > {
> > > int cpu = smp_processor_id();
> > >
> > > - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> > > + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
> >
> > Sorry for replying "too late", but grepping through the kernel code I
> > fail to find any caller that does not dereference vma before calling
> > (local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
> > be NULL, so I would say it is safe to assume vma is never NULL, and
> > the NULL check can be removed completely.
> >
> > Also it looks like this "bug" was there since at least 2.6.12, and
> > never seem to have bitten anyone.
>
> Yes, the bug pre-dates GIT history and I agree that it is most unlikely
> that it ever caused a problem. I will send a new patch that removes the
> NULL check of vma.
Well, as at 2.4.26 (picked randomly) code in arch/mips/mm/tlb-r4k.c had
the same check:
void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
{
int cpu = smp_processor_id();
if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
[...]
but code in arch/mips64/mm/tlb-r4k.c did not:
void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
{
int cpu = smp_processor_id();
if (cpu_context(cpu, vma->vm_mm) != 0) {
unsigned long flags;
[...]
and the current situation is an artefact from the 32-bit and 64-bit port
unification:
commit efc2f9a8a100511399309a50a33b46ff099f3454
Author: Ralf Baechle <[email protected]>
Date: Mon Jul 21 03:03:15 2003 +0000
Unify tlb-r4k.c.
-- at which point tlb-r3k.c wasn't updated accordingly. The condition was
eventually removed from arch/mips/mm/tlb-r4k.c in 2.4 too with:
commit fd8917812546ef2278e006237a7cc38497bfbf52
Author: Ralf Baechle <[email protected]>
Date: Thu Nov 25 22:18:38 2004 +0000
Try to glue the hazard avoidance stuff the same way it was done for
2.6 before the TLB synthesizer. Previously the code for some CPUs
such as the RM9000 was completly bogus nonsense ... I guess we may
eventually want to backport the tlb synthesizer to 2.4 once the dust
has settled.
And arch/mips64/mm/tlb-r4k.c was introduced with:
commit 49be6d9f37bbac0a0c413a2a63dcf7cc497d144a
Author: Ralf Baechle <[email protected]>
Date: Wed Jul 24 16:12:03 2002 +0000
Random 64-bit updates and fixes.
and never had the condition in the first place.
We do have full 2.6 GIT history, beyond 2.6.12. It seems cut off at
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 for some operations, but not
plain `git log' for example, and I was able to peek beyond that by
checking out the immediately preceding commit, that is
66f0a432564b5f0ebf632263ceac84a10a99de09:
commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
Author: Linus Torvalds <[email protected]>
Date: Sat Apr 16 15:20:36 2005 -0700
Linux-2.6.12-rc2
Initial git repository build. I'm not bothering with the full history,
even though we have it. We can create a separate "historical" git
archive of that later if we want to, and in the meantime it's about
3.2GB when imported into git - space that would just make the early
git days unnecessarily complicated, when we don't have a lot of good
infrastructure for it.
Let it rip!
commit 66f0a432564b5f0ebf632263ceac84a10a99de09
Author: Ralf Baechle <[email protected]>
Date: Fri Apr 15 14:40:46 2005 +0000
Add resource managment.
There may be a better way, I don't claim GIT expertise.
Maciej
Hello Maciej,
I didn't know it was possible to go beyond commit 1da177e,
thank you for the info.
Best regards,
Emil
On Sun, Jul 06, 2014 at 12:16:38PM +0100, Maciej W. Rozycki wrote:
> On Sun, 6 Jul 2014, Emil Goode wrote:
>
> > > > diff --git a/arch/mips/mm/tlb-r3k.c b/arch/mips/mm/tlb-r3k.c
> > > > index d657493..6546758 100644
> > > > --- a/arch/mips/mm/tlb-r3k.c
> > > > +++ b/arch/mips/mm/tlb-r3k.c
> > > > @@ -158,7 +158,7 @@ void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> > > > {
> > > > int cpu = smp_processor_id();
> > > >
> > > > - if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> > > > + if (vma && cpu_context(cpu, vma->vm_mm) != 0) {
> > >
> > > Sorry for replying "too late", but grepping through the kernel code I
> > > fail to find any caller that does not dereference vma before calling
> > > (local)flush_tlb_page(). Also both tlb-4k and tlb-8k assume vma cannot
> > > be NULL, so I would say it is safe to assume vma is never NULL, and
> > > the NULL check can be removed completely.
> > >
> > > Also it looks like this "bug" was there since at least 2.6.12, and
> > > never seem to have bitten anyone.
> >
> > Yes, the bug pre-dates GIT history and I agree that it is most unlikely
> > that it ever caused a problem. I will send a new patch that removes the
> > NULL check of vma.
>
> Well, as at 2.4.26 (picked randomly) code in arch/mips/mm/tlb-r4k.c had
> the same check:
>
> void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> {
> int cpu = smp_processor_id();
>
> if (!vma || cpu_context(cpu, vma->vm_mm) != 0) {
> unsigned long flags;
> [...]
>
> but code in arch/mips64/mm/tlb-r4k.c did not:
>
> void local_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
> {
> int cpu = smp_processor_id();
>
> if (cpu_context(cpu, vma->vm_mm) != 0) {
> unsigned long flags;
> [...]
>
> and the current situation is an artefact from the 32-bit and 64-bit port
> unification:
>
> commit efc2f9a8a100511399309a50a33b46ff099f3454
> Author: Ralf Baechle <[email protected]>
> Date: Mon Jul 21 03:03:15 2003 +0000
>
> Unify tlb-r4k.c.
>
> -- at which point tlb-r3k.c wasn't updated accordingly. The condition was
> eventually removed from arch/mips/mm/tlb-r4k.c in 2.4 too with:
>
> commit fd8917812546ef2278e006237a7cc38497bfbf52
> Author: Ralf Baechle <[email protected]>
> Date: Thu Nov 25 22:18:38 2004 +0000
>
> Try to glue the hazard avoidance stuff the same way it was done for
> 2.6 before the TLB synthesizer. Previously the code for some CPUs
> such as the RM9000 was completly bogus nonsense ... I guess we may
> eventually want to backport the tlb synthesizer to 2.4 once the dust
> has settled.
>
> And arch/mips64/mm/tlb-r4k.c was introduced with:
>
> commit 49be6d9f37bbac0a0c413a2a63dcf7cc497d144a
> Author: Ralf Baechle <[email protected]>
> Date: Wed Jul 24 16:12:03 2002 +0000
>
> Random 64-bit updates and fixes.
>
> and never had the condition in the first place.
>
> We do have full 2.6 GIT history, beyond 2.6.12. It seems cut off at
> 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 for some operations, but not
> plain `git log' for example, and I was able to peek beyond that by
> checking out the immediately preceding commit, that is
> 66f0a432564b5f0ebf632263ceac84a10a99de09:
>
> commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2
> Author: Linus Torvalds <[email protected]>
> Date: Sat Apr 16 15:20:36 2005 -0700
>
> Linux-2.6.12-rc2
>
> Initial git repository build. I'm not bothering with the full history,
> even though we have it. We can create a separate "historical" git
> archive of that later if we want to, and in the meantime it's about
> 3.2GB when imported into git - space that would just make the early
> git days unnecessarily complicated, when we don't have a lot of good
> infrastructure for it.
>
> Let it rip!
>
> commit 66f0a432564b5f0ebf632263ceac84a10a99de09
> Author: Ralf Baechle <[email protected]>
> Date: Fri Apr 15 14:40:46 2005 +0000
>
> Add resource managment.
>
> There may be a better way, I don't claim GIT expertise.
>
> Maciej