linux-kernel - Re: [PATCH] fs: fix i_writecount on shmem and friends

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: <CANq1E4QSFjDiYVmfrNJFfK6zW_HhSBd5d0QF-2t48DzUP+B6pw@mail.gmail.com>
Date:	Wed, 12 Mar 2014 23:30:09 +0100
From:	David Herrmann <dh.herrmann@...il.com>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Howells <dhowells@...hat.com>,
	Oleg Nesterov <oleg@...hat.com>,
	stable <stable@...r.kernel.org>, Neil Brown <neilb@...e.de>
Subject: Re: [PATCH] fs: fix i_writecount on shmem and friends

Hi

> I think it's trying to fix the problem in the wrong place.  The bug is real,
> all right, but it's not that alloc_file() for non-regulars doesn't grab
> writecount; it's that drop_file_write_access() drops it for those.
>
> What the hell would we want to play with that counter for, anyway?  It's not
> as if they could be mmapped, so all it does is making pipe(2) and socket(2)
> more costly, for no visible reason.

Please see:
  shmem_zero_setup()
    shmem_file_setup()
      __shmem_file_setup()
        alloc_file()

shmem_zero_setup() is used by /dev/zero (drivers/char/mem.c) and
mmap(MAP_ANON). So yes, we do call mmap() on these files.
I also disagree on "for no visible reason". The reason to do this is
uniformity. We make i_writecount work on all inodes regardless how
they got created. Breaking consistent behavior just to save an
atomic_inc_unless_negative() in the alloc_file() path seems
unreasonable to me.

Anyhow, I haven't found any bug if we follow your recommendation.
Every path using alloc_file() either prevents write access on the
underlying inode (eg., anon-inode) or prevents user-space from getting
an FD (all the shmem_file_setup() paths). So it's up to you. Feel free
to fix it yourself, otherwise I will send a second patch following
your idea tomorrow.

Oh, and yes, the md stuff is broken, but I thought that's an orthogonal issue..

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/