| Message ID | 20180730143710.14413-2-christian@brauner.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | file: export functions for binder module | expand |
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.
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.
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.
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
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
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
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) {
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(+)