From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753319AbcG2RfL (ORCPT ); Fri, 29 Jul 2016 13:35:11 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34941 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753098AbcG2RfH (ORCPT ); Fri, 29 Jul 2016 13:35:07 -0400 From: Nicolai Stange To: Liviu Dudau Cc: Greg Kroah-Hartman , Brian Starkey , LKML , Nicolai Stange Subject: Re: [PATCH] debugfs: Add proxy function for the mmap file operation References: <20160729143459.2672-1-Liviu.Dudau@arm.com> Date: Fri, 29 Jul 2016 19:35:02 +0200 In-Reply-To: <20160729143459.2672-1-Liviu.Dudau@arm.com> (Liviu Dudau's message of "Fri, 29 Jul 2016 15:34:59 +0100") Message-ID: <87oa5gpcu1.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.95 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Liviu Dudau writes: > Add proxy function for the mmap file_operations hook under the > full_proxy_fops structure. This is useful for providing a custom > mmap routine in a driver's debugfs implementation. I guess you've got some specific use case for mmap() usage on some new debugfs file in mind? Currently, there exist only two mmap providers: drivers/staging/android/sync_debug.c kernel/kcov.c Both don't suffer from the lack of mmap support in the debugfs full proxy implementation because they don't use it -- their files never go away and thus, can be (and are) created via debugfs_create_file_unsafe(). However, if you wish to have some mmapable debugfs file which *can* go away, introducing mmap support in the debugfs full proxy is perfectly valid. But please see below. > Cc: Nicolai Stange > Signed-off-by: Liviu Dudau > --- > fs/debugfs/file.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c > index 592059f..d87148a 100644 > --- a/fs/debugfs/file.c > +++ b/fs/debugfs/file.c > @@ -168,6 +168,10 @@ FULL_PROXY_FUNC(write, ssize_t, filp, > loff_t *ppos), > ARGS(filp, buf, size, ppos)); > > +FULL_PROXY_FUNC(mmap, int, filp, > + PROTO(struct file *filp, struct vm_area_struct *vma), > + ARGS(filp, vma)); > + While this protects the call to ->mmap() itself against file removal races, it doesn't protect anything possibly installed at vma->vm_ops from that. I'm fine with this as long as vma->vm_ops isn't set from ->mmap() ;). At the very least, we should probably provide a Coccinelle script for this. I'll try to put something together at the weekend or at the beginning of next week (if you aren't faster). Another option would be to add a check in the wrapping ->mmap() whether the vma->vm_ops has been set from the wrapped ->mmap(). Greg, do you think such a runtime check would be a good thing to have? Btw, it would certainly be possible to even support a custom vma->vm_ops by proxying this one, too. However, we probably would have to SIGSEGV userspace if ->fault() was called on a stale debugfs file. And since nobody has asked for this feature yet, I don't think that it should be implemented now. Thanks, Nicolai