mbox series

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

Message ID 20180730143710.14413-1-christian@brauner.io (mailing list archive)
Headers show
Series file: export functions for binder module | expand

Message

Christian Brauner July 30, 2018, 2:37 p.m. UTC 
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(+)

Comments

Christoph Hellwig July 30, 2018, 4:34 p.m. UTC | #1 
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.
Christian Brauner July 30, 2018, 8:12 p.m. UTC | #2 
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
Matthew Wilcox July 30, 2018, 8:19 p.m. UTC | #3 
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.
Christian Brauner July 30, 2018, 8:28 p.m. UTC | #4 
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
Al Viro July 30, 2018, 9:41 p.m. UTC | #5 
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).
Ben Hutchings July 31, 2018, 2:27 p.m. UTC | #6 
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.
Christian Brauner Aug. 2, 2018, 1:31 p.m. UTC | #7 
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