linux-kernel - Re: [GIT PULL] parisc updates for v3.15

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFw+0_Wf+_2EAdi=OhVK3cZdMwsCV5cditaLz7LJroeByA@mail.gmail.com>
Date:	Thu, 1 May 2014 12:23:09 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Helge Deller <deller@....de>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Parisc List <linux-parisc@...r.kernel.org>,
	James Bottomley <James.Bottomley@...senpartnership.com>
Subject: Re: [GIT PULL] parisc updates for v3.15

On Thu, May 1, 2014 at 11:38 AM, Helge Deller <deller@....de> wrote:
>
> - Make mmap() behave similiar to other architectures: If a file hasn't been
>   mapped yet, we can now map it at any given page-aligned address.

Ugh, so I pulled this, but I'm going to unpull it, because I dislike
your new "i_mmap_lastmap" field.

Now, the i_mmap_lastmap field itself, I could probably live with, but
this change to generic code:

  +       if (vma->vm_flags & VM_SHARED) {
                  mapping->i_mmap_writable--;
  +#ifdef CONFIG_MMAP_TRACKING
  +               if (mapping->i_mmap_writable == 0)
  +                       mapping->i_mmap_lastmmap = 0;
  +#endif
  +       }

makes me just gouge my eyes out. It's not only uglifying generic code,
it's _stupid_ even when it's used.

What's magic about "i_mmap_lastmmap" having the value zero? Nothing.
Maybe somebody wants to map stuff at that zero value, and has the
permissions to do so.

So zeroing it is wrong. It's also entirely unnecessary, since you can
just look at the "mapping->i_mmap_writable" value instead. So instead
of checking "is i_mmap_lastmmap zero" as a way to check whether you
can now use any virtual address, which is wrong _anyway_, you should
have checked "is i_mmap_writable zero".

Now, I *also* suspect that you could just get rid of i_mmap_lastmmap
_entirely_, and instead just make "GET_LAST_MMAP()" just look up the
first shared mapping it can find in the rb tree (if i_mmap_writable is
non-null).

But if it was just that one (unnecessary) field in the "struct
address_space", I probably wouldn't mind, and say "ok, parisc has
broken virtual caches, what else is new". But the fact that it adds
code to the generic file just adds insult to injury and makes me go
"no, I don't want to pull this".

Mind fixing this? Here's a TOTALLY UNTESTED patch that may or may not
work, but might at least act as a starting point.

Hmm?

              Linus

View attachment "patch.diff" of type "text/plain" (2014 bytes)