[RFC PATCH 0/4] file: export functions for binder module

[RFC PATCH 0/4] file: export functions for binder module

[Christian Brauner]

Hey,

We currently plan on turning the Android binder and ashmem driver into a
module. We have seen more and more requests by users to be able to use
the binder and ashmem features without wanting to convince each distro
to enable it by default in their kernel. Debian already started to carry
patches for turning them into modules.
The main problem is that binder currently uses multiple functions that
are not exported and are pretty low-level. The most obvious ones that
fall into this category are __alloc_fd(), __fd_install(),
get_files_struct(), and put_files_struct(). Being an IPC mechanism
binder seems like a reasonable user of these functions.
I don't expect this patch to be mergeable but rather to kick-off a
discussion if we can either simply export them as they are or how we can
get supportable exports that allow access to struct files_struct.

Thanks!
Christian

Christian Brauner (4):
  file: export __alloc_fd()
  file: export __fd_install()
  file: export get_files_struct()
  file: export put_files_struct()

 fs/file.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.17.1

[PATCH 1/4] file: export __alloc_fd()

[Christian Brauner]

The Android binder driver will be turned into a module. Since it uses
__alloc_fd() we need to export this function.

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Todd Kjos <tkjos@android.com>
Cc: Robert Love <rlove@google.com>
Cc: Ben Hutching <ben@decadent.org.uk>
Cc: Martijn Coenen <maco@android.com>
Cc: Arve Hjønnevåg <arve@android.com>
---
 fs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index 7ffd6e9d103d..90382c3ac6e7 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -533,6 +533,7 @@ int __alloc_fd(struct files_struct *files,
 	spin_unlock(&files->file_lock);
 	return error;
 }
+EXPORT_SYMBOL(__alloc_fd);
 
 static int alloc_fd(unsigned start, unsigned flags)
 {
-- 
2.17.1

[PATCH 2/4] file: export __fd_install()

[Christian Brauner]

The Android binder driver will be turned into a module. Since it
uses __fd_install() we need to export this function.

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Todd Kjos <tkjos@android.com>
Cc: Robert Love <rlove@google.com>
Cc: Ben Hutching <ben@decadent.org.uk>
Cc: Martijn Coenen <maco@android.com>
Cc: Arve Hjønnevåg <arve@android.com>
---
 fs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index 90382c3ac6e7..b1ffa7acff69 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -607,6 +607,7 @@ void __fd_install(struct files_struct *files, unsigned int fd,
 	rcu_assign_pointer(fdt->fd[fd], file);
 	rcu_read_unlock_sched();
 }
+EXPORT_SYMBOL(__fd_install);
 
 void fd_install(unsigned int fd, struct file *file)
 {
-- 
2.17.1

[PATCH 3/4] file: export get_files_struct()

[Christian Brauner]

The Android binder driver will be turned into a module. Since it uses
get_files_struct() we need to export this function.

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Todd Kjos <tkjos@android.com>
Cc: Robert Love <rlove@google.com>
Cc: Ben Hutching <ben@decadent.org.uk>
Cc: Martijn Coenen <maco@android.com>
Cc: Arve Hjønnevåg <arve@android.com>
---
 fs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index b1ffa7acff69..430d74642e00 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -409,6 +409,7 @@ struct files_struct *get_files_struct(struct task_struct *task)
 
 	return files;
 }
+EXPORT_SYMBOL(get_files_struct);
 
 void put_files_struct(struct files_struct *files)
 {
-- 
2.17.1

[PATCH 4/4] file: export put_files_struct()

[Christian Brauner]

The Android binder driver will be turned into a module. Since it uses
put_files_struct() we need to export this function.

Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Todd Kjos <tkjos@android.com>
Cc: Robert Love <rlove@google.com>
Cc: Ben Hutching <ben@decadent.org.uk>
Cc: Martijn Coenen <maco@android.com>
Cc: Arve Hjønnevåg <arve@android.com>
---
 fs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/file.c b/fs/file.c
index 430d74642e00..89a62165c5a0 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -422,6 +422,7 @@ void put_files_struct(struct files_struct *files)
 		kmem_cache_free(files_cachep, files);
 	}
 }
+EXPORT_SYMBOL(put_files_struct);
 
 void reset_files_struct(struct files_struct *files)
 {
-- 
2.17.1

Re: [PATCH 1/4] file: export __alloc_fd()

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 04:37:07PM +0200, Christian Brauner wrote:
> The Android binder driver will be turned into a module. Since it uses
> __alloc_fd() we need to export this function.

Err, hell no.

It should be using an anon fd probably.

Re: [PATCH 2/4] file: export __fd_install()

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 04:37:08PM +0200, Christian Brauner wrote:
> The Android binder driver will be turned into a module. Since it
> uses __fd_install() we need to export this function.

Same as the previous one.  No driver should be pooking this deep
into fd internals (not even non-modular ones for that matter).

Re: [PATCH 3/4] file: export get_files_struct()

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 04:37:09PM +0200, Christian Brauner wrote:
> The Android binder driver will be turned into a module. Since it uses
> get_files_struct() we need to export this function.

Hell no.  Please explain why so that we can find a better way.

Re: [PATCH 4/4] file: export put_files_struct()

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 04:37:10PM +0200, Christian Brauner wrote:
> The Android binder driver will be turned into a module. Since it uses
> put_files_struct() we need to export this function.

Same as above.

Re: [RFC PATCH 0/4] file: export functions for binder module

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 04:37:06PM +0200, Christian Brauner wrote:
> Hey,
> 
> We currently plan on turning the Android binder and ashmem driver into a
> module. We have seen more and more requests by users to be able to use
> the binder and ashmem features without wanting to convince each distro
> to enable it by default in their kernel. Debian already started to carry
> patches for turning them into modules.

Yikes.  I really wish Debian would stick more to upstream rather than
picking random crap like this up.

Re: [RFC PATCH 0/4] file: export functions for binder module

[Christian Brauner]

On Mon, Jul 30, 2018 at 09:34:52AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 30, 2018 at 04:37:06PM +0200, Christian Brauner wrote:
> > Hey,
> > 
> > We currently plan on turning the Android binder and ashmem driver into a
> > module. We have seen more and more requests by users to be able to use
> > the binder and ashmem features without wanting to convince each distro
> > to enable it by default in their kernel. Debian already started to carry
> > patches for turning them into modules.
> 
> Yikes.  I really wish Debian would stick more to upstream rather than
> picking random crap like this up.

Thanks for the review, Christoph. Unfortunately the gist of the message
got cut off:

> I don't expect this patch to be mergeable but rather to kick-off a
> discussion if we can either simply export them as they are or how we can
> get supportable exports that allow access to struct files_struct.

Maybe that wasn't obvious from the first message. Is there any way we
can come up with a way to have versions of these functions that you
would be fine with exporting?
The point is that otherwise we would have to either duplicate the code
or come up with something way more complex. If you have any pointer that
would already help.

Christian

Re: [RFC PATCH 0/4] file: export functions for binder module

[Matthew Wilcox]

On Mon, Jul 30, 2018 at 10:12:24PM +0200, Christian Brauner wrote:
> > I don't expect this patch to be mergeable but rather to kick-off a
> > discussion if we can either simply export them as they are or how we can
> > get supportable exports that allow access to struct files_struct.
> 
> Maybe that wasn't obvious from the first message. Is there any way we
> can come up with a way to have versions of these functions that you
> would be fine with exporting?
> The point is that otherwise we would have to either duplicate the code
> or come up with something way more complex. If you have any pointer that
> would already help.

He said in the first reply this should probably be using an anonfd.
If you do that, I think all four of these exports go away.

And there was really no reason to post each of the four exports as
separate patches.  That just makes review harder on everyone.

Re: [RFC PATCH 0/4] file: export functions for binder module

[Christian Brauner]

On Mon, Jul 30, 2018 at 01:19:47PM -0700, Matthew Wilcox wrote:
> On Mon, Jul 30, 2018 at 10:12:24PM +0200, Christian Brauner wrote:
> > > I don't expect this patch to be mergeable but rather to kick-off a
> > > discussion if we can either simply export them as they are or how we can
> > > get supportable exports that allow access to struct files_struct.
> > 
> > Maybe that wasn't obvious from the first message. Is there any way we
> > can come up with a way to have versions of these functions that you
> > would be fine with exporting?
> > The point is that otherwise we would have to either duplicate the code
> > or come up with something way more complex. If you have any pointer that
> > would already help.
> 
> He said in the first reply this should probably be using an anonfd.
> If you do that, I think all four of these exports go away.

I try and see if that is possible.

> 
> And there was really no reason to post each of the four exports as
> separate patches.  That just makes review harder on everyone.

Sorry about that. It usually depends on the preferences of each
maintainer how fine-grained such minor changes should be.

Christian

Re: [PATCH 1/4] file: export __alloc_fd()

[Matthew Wilcox]

On Mon, Jul 30, 2018 at 09:31:55AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 30, 2018 at 04:37:07PM +0200, Christian Brauner wrote:
> > The Android binder driver will be turned into a module. Since it uses
> > __alloc_fd() we need to export this function.
> 
> Err, hell no.
> 
> It should be using an anon fd probably.

I'm not entirely sure I understand the binder code (... does anyone?)
but from what I can see, it intends to open a file descriptor in the
process which is the target of the message being sent.  That strikes
me as wrong-headed; it should be allocating a struct file and passing
that file to the other process.  When that process receives the message,
*it* allocates a file descriptor for itself.

But I think the binder user-space API relies on this.  The userspace API
seems to rely on passing fd numbers around ... but I'm having trouble
figuring most of this user API out.  Perhaps Martijn can help here.

Re: [RFC PATCH 0/4] file: export functions for binder module

[Al Viro]

On Mon, Jul 30, 2018 at 10:28:40PM +0200, Christian Brauner wrote:
> On Mon, Jul 30, 2018 at 01:19:47PM -0700, Matthew Wilcox wrote:
> > On Mon, Jul 30, 2018 at 10:12:24PM +0200, Christian Brauner wrote:
> > > > I don't expect this patch to be mergeable but rather to kick-off a
> > > > discussion if we can either simply export them as they are or how we can
> > > > get supportable exports that allow access to struct files_struct.
> > > 
> > > Maybe that wasn't obvious from the first message. Is there any way we
> > > can come up with a way to have versions of these functions that you
> > > would be fine with exporting?
> > > The point is that otherwise we would have to either duplicate the code
> > > or come up with something way more complex. If you have any pointer that
> > > would already help.
> > 
> > He said in the first reply this should probably be using an anonfd.
> > If you do that, I think all four of these exports go away.
> 
> I try and see if that is possible.
> 
> > 
> > And there was really no reason to post each of the four exports as
> > separate patches.  That just makes review harder on everyone.
> 
> Sorry about that. It usually depends on the preferences of each
> maintainer how fine-grained such minor changes should be.

The fundamental problem here (besides "who the hell thought that this Fine Piece
Of Software belongs anywhere other than in /dev/null?") is that messing with
other's descriptor table is Fucking Wrong(tm).  It's not going to become
a general-purpose interface.  That kludge is just that - a kludge caused by
atrocious API design.

Exports NAKed, and if brought again they'll get NAKed with extreme prejudice
(sensu PTerry).

Re: [PATCH 1/4] file: export __alloc_fd()

[Al Viro]

On Mon, Jul 30, 2018 at 01:36:33PM -0700, Matthew Wilcox wrote:
> On Mon, Jul 30, 2018 at 09:31:55AM -0700, Christoph Hellwig wrote:
> > On Mon, Jul 30, 2018 at 04:37:07PM +0200, Christian Brauner wrote:
> > > The Android binder driver will be turned into a module. Since it uses
> > > __alloc_fd() we need to export this function.
> > 
> > Err, hell no.
> > 
> > It should be using an anon fd probably.
> 
> I'm not entirely sure I understand the binder code (... does anyone?)
> but from what I can see, it intends to open a file descriptor in the
> process which is the target of the message being sent.  That strikes
> me as wrong-headed; it should be allocating a struct file and passing
> that file to the other process.  When that process receives the message,
> *it* allocates a file descriptor for itself.
> 
> But I think the binder user-space API relies on this.  The userspace API
> seems to rely on passing fd numbers around ... but I'm having trouble
> figuring most of this user API out.  Perhaps Martijn can help here.

... and there's a perfectly sane solution to that - it's called git rm.

Re: [PATCH 1/4] file: export __alloc_fd()

[Martijn Coenen]

On Mon, Jul 30, 2018 at 10:36 PM, Matthew Wilcox <willy@infradead.org> wrote:
> I'm not entirely sure I understand the binder code (... does anyone?)
> but from what I can see, it intends to open a file descriptor in the
> process which is the target of the message being sent.

You're right.

> That strikes
> me as wrong-headed; it should be allocating a struct file and passing
> that file to the other process.  When that process receives the message,
> *it* allocates a file descriptor for itself.ho

We're looking into cleaning this up (historically it was done this way
because VIVT caches made this not very efficient), and this is indeed
a very good candidate for fixing it.

>
> But I think the binder user-space API relies on this.  The userspace API
> seems to rely on passing fd numbers around ... but I'm having trouble
> figuring most of this user API out.  Perhaps Martijn can help here.

The UAPI does expect a file descriptor, but we may be able to do the
mapping from fd to struct file (and vice-versa) in the kernel driver,
so userspace wouldn't notice.

Thanks,
Martijn

Re: [PATCH 1/4] file: export __alloc_fd()

[Christian Brauner]

On Tue, Jul 31, 2018 at 10:44:33AM +0200, Martijn Coenen wrote:
> On Mon, Jul 30, 2018 at 10:36 PM, Matthew Wilcox <willy@infradead.org> wrote:
> > I'm not entirely sure I understand the binder code (... does anyone?)
> > but from what I can see, it intends to open a file descriptor in the
> > process which is the target of the message being sent.
> 
> You're right.
> 
> > That strikes
> > me as wrong-headed; it should be allocating a struct file and passing
> > that file to the other process.  When that process receives the message,
> > *it* allocates a file descriptor for itself.ho
> 
> We're looking into cleaning this up (historically it was done this way
> because VIVT caches made this not very efficient), and this is indeed
> a very good candidate for fixing it.

Ah, this wasn't brought up in the original thread when discussing to
turn this into a module. If using internal functions like this is going
away it makes sense to wait for this work to happen first. Is there a
time-frame for this?

Thanks!
Christian

> 
> >
> > But I think the binder user-space API relies on this.  The userspace API
> > seems to rely on passing fd numbers around ... but I'm having trouble
> > figuring most of this user API out.  Perhaps Martijn can help here.
> 
> The UAPI does expect a file descriptor, but we may be able to do the
> mapping from fd to struct file (and vice-versa) in the kernel driver,
> so userspace wouldn't notice.
> 
> Thanks,
> Martijn

Re: [PATCH 1/4] file: export __alloc_fd()

[Martijn Coenen]

On Tue, Jul 31, 2018 at 12:07 PM, Christian Brauner
<christian@brauner.io> wrote:
> Ah, this wasn't brought up in the original thread when discussing to
> turn this into a module. If using internal functions like this is going
> away it makes sense to wait for this work to happen first. Is there a
> time-frame for this?

Not yet, it depends on the solution and who has cycles to work on it.

Martijn

>
> Thanks!
> Christian
>
>>
>> >
>> > But I think the binder user-space API relies on this.  The userspace API
>> > seems to rely on passing fd numbers around ... but I'm having trouble
>> > figuring most of this user API out.  Perhaps Martijn can help here.
>>
>> The UAPI does expect a file descriptor, but we may be able to do the
>> mapping from fd to struct file (and vice-versa) in the kernel driver,
>> so userspace wouldn't notice.
>>
>> Thanks,
>> Martijn

Re: [RFC PATCH 0/4] file: export functions for binder module

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Mon, 2018-07-30 at 09:34 -0700, Christoph Hellwig wrote:
> On Mon, Jul 30, 2018 at 04:37:06PM +0200, Christian Brauner wrote:
> > Hey,
> > 
> > We currently plan on turning the Android binder and ashmem driver into a
> > module. We have seen more and more requests by users to be able to use
> > the binder and ashmem features without wanting to convince each distro
> > to enable it by default in their kernel. Debian already started to carry
> > patches for turning them into modules.
> 
> Yikes.  I really wish Debian would stick more to upstream rather than
> picking random crap like this up.

My hope is that this is a temporary bodge.

The way this happened was:

1. Anbox was proposed as an addition to Debian, including the ashmem
and binder drivers as out-of-tree modules.
2. It was objected that these drivers were already part of the linux
package (though not currently built), and it was bad practice to add a
second copy that would need separate security updates.
3. The kernel team was requested to enable these drivers.
4. I agreed to enable them as modules (like most other drivers).
5. I then discovered that they couldn't be built as modules without
patching, due to these missing exports.

(So how does Anbox build them as modules?  Well, you're really not
going to like this:
https://github.com/anbox/anbox-modules/blob/master/binder/deps.c )

Ben.

-- 
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought. I realized that a large part of my life from then on was going
to be spent in finding mistakes in my own programs.
                                                 - Maurice Wilkes, 1949

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/4] file: export functions for binder module

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

On Mon, Jul 30, 2018 at 10:41:09PM +0100, Al Viro wrote:
> On Mon, Jul 30, 2018 at 10:28:40PM +0200, Christian Brauner wrote:
> > On Mon, Jul 30, 2018 at 01:19:47PM -0700, Matthew Wilcox wrote:
> > > On Mon, Jul 30, 2018 at 10:12:24PM +0200, Christian Brauner wrote:
> > > > > I don't expect this patch to be mergeable but rather to kick-off a
> > > > > discussion if we can either simply export them as they are or how we can
> > > > > get supportable exports that allow access to struct files_struct.
> > > > 
> > > > Maybe that wasn't obvious from the first message. Is there any way we
> > > > can come up with a way to have versions of these functions that you
> > > > would be fine with exporting?
> > > > The point is that otherwise we would have to either duplicate the code
> > > > or come up with something way more complex. If you have any pointer that
> > > > would already help.
> > > 
> > > He said in the first reply this should probably be using an anonfd.
> > > If you do that, I think all four of these exports go away.
> > 
> > I try and see if that is possible.
> > 
> > > 
> > > And there was really no reason to post each of the four exports as
> > > separate patches.  That just makes review harder on everyone.
> > 
> > Sorry about that. It usually depends on the preferences of each
> > maintainer how fine-grained such minor changes should be.
> 
> The fundamental problem here (besides "who the hell thought that this Fine Piece
> Of Software belongs anywhere other than in /dev/null?") is that messing with
> other's descriptor table is Fucking Wrong(tm).  It's not going to become
> a general-purpose interface.  That kludge is just that - a kludge caused by
> atrocious API design.
> 
> Exports NAKed, and if brought again they'll get NAKed with extreme prejudice

That's fair. When this discussion of turning them into modules was
started it was expected that this would never fly as is. The question
was whether there's any way for binder to touch struct_files of another
process directly at all. Now we know, there isn't. What I was hoping for
is that this would cause a redesign that avoids touching these helpers.
There's an effort from the Android folks now, which is good!

Thanks!
Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]