Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180828160005.90796-1-tkjos@google.com> <20180829070036.GA13718@infradead.org>
In-Reply-To: <20180829070036.GA13718@infradead.org>
From:   Todd Kjos <tkjos@google.com>
Date:   Thu, 30 Aug 2018 08:49:40 -0700
Message-ID: <CAHRSSExgXbKOYR9HU=+WDaQjHBYB3=YCYXgnAOGwJ0Y3Q3qshw@mail.gmail.com>
Subject: Re: [PATCH] binder: use standard functions to allocate fds
To:     hch@infradead.org
Cc:     Todd Kjos <tkjos@android.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Martijn Coenen <maco@google.com>,
        Christian Brauner <christian.brauner@canonical.com>,
        ben@decadent.org.uk, torvalds@linux-foundation.org,
        Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 29, 2018 at 12:00 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> >  config ANDROID_BINDER_IPC
> >       bool "Android Binder IPC Driver"
> > -     depends on MMU
> > +     depends on MMU && !CPU_CACHE_VIVT
>
> Thats is a purely arm specific symbol which should not be
> used in common code.  Nevermind that there generally should
> be no good reason for it.

It is true that the current design (10+ years old) has this flaw. VIVT
cache support was the rationale for using the non-standard functions
to allocate file descriptors from the sender context. There are no
known cases of recent android releases running on a device with
a VIVT cache architecture, but I did want to call this out.

We're working on refactoring to eliminate this issue.

>
> > +     fixup->offset = (uintptr_t)fdp - (uintptr_t)t->buffer->data;
>
> This looks completely broken.  Why would you care at what exact
> place the fd is placed?  Oh, because you share an array with fds
> with userspace, which is a hell of a bad idea, and then maninpulate
> that buffer mapped to userspace from kernel threads.

Well, android apps rely on this behavior (and have for 10+ years) so
there is no way to avoid the need to manipulate the buffer from
the kernel. We are working to refactor the driver to do this
using standard mechanisms instead of relying on low-level internal
functions.

>
> I think we just need to rm -rf drivers/android/binder*.c and be done
> with it, as this piece of crap should never have been merged to start
> with.