| Message ID | 20230417205631.1956027-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: reduce ioctl stack usage | expand |
On 4/18/23 4:56 AM, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > On 32-bit architectures with KASAN_STACK enabled, the total stack usage > of the ocfs2_ioctl function grows beyond the warning limit: > > fs/ocfs2/ioctl.c: In function 'ocfs2_ioctl': > fs/ocfs2/ioctl.c:934:1: error: the frame size of 1448 bytes is larger than 1400 bytes [-Werror=frame-larger-than=] > > Move each of the variables into a basic block, and mark ocfs2_info_handle() > as noinline_for_stack, in order to have the variable share stack slots. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Looks good. Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com> > --- > fs/ocfs2/ioctl.c | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c > index 811a6ea374bb..b1550ba73f96 100644 > --- a/fs/ocfs2/ioctl.c > +++ b/fs/ocfs2/ioctl.c > @@ -803,8 +803,8 @@ static int ocfs2_get_request_ptr(struct ocfs2_info *info, int idx, > * a better backward&forward compatibility, since a small piece of > * request will be less likely to be broken if disk layout get changed. > */ > -static int ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, > - int compat_flag) > +static noinline_for_stack int > +ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, int compat_flag) > { > int i, status = 0; > u64 req_addr; > @@ -840,27 +840,26 @@ static int ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, > long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > { > struct inode *inode = file_inode(filp); > - int new_clusters; > - int status; > - struct ocfs2_space_resv sr; > - struct ocfs2_new_group_input input; > - struct reflink_arguments args; > - const char __user *old_path; > - const char __user *new_path; > - bool preserve; > - struct ocfs2_info info; > void __user *argp = (void __user *)arg; > + int status; > > switch (cmd) { > case OCFS2_IOC_RESVSP: > case OCFS2_IOC_RESVSP64: > case OCFS2_IOC_UNRESVSP: > case OCFS2_IOC_UNRESVSP64: > + { > + struct ocfs2_space_resv sr; > + > if (copy_from_user(&sr, (int __user *) arg, sizeof(sr))) > return -EFAULT; > > return ocfs2_change_file_space(filp, cmd, &sr); > + } > case OCFS2_IOC_GROUP_EXTEND: > + { > + int new_clusters; > + > if (!capable(CAP_SYS_RESOURCE)) > return -EPERM; > > @@ -873,8 +872,12 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > status = ocfs2_group_extend(inode, new_clusters); > mnt_drop_write_file(filp); > return status; > + } > case OCFS2_IOC_GROUP_ADD: > case OCFS2_IOC_GROUP_ADD64: > + { > + struct ocfs2_new_group_input input; > + > if (!capable(CAP_SYS_RESOURCE)) > return -EPERM; > > @@ -887,7 +890,14 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > status = ocfs2_group_add(inode, &input); > mnt_drop_write_file(filp); > return status; > + } > case OCFS2_IOC_REFLINK: > + { > + struct reflink_arguments args; > + const char __user *old_path; > + const char __user *new_path; > + bool preserve; > + > if (copy_from_user(&args, argp, sizeof(args))) > return -EFAULT; > old_path = (const char __user *)(unsigned long)args.old_path; > @@ -895,11 +905,16 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > preserve = (args.preserve != 0); > > return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve); > + } > case OCFS2_IOC_INFO: > + { > + struct ocfs2_info info; > + > if (copy_from_user(&info, argp, sizeof(struct ocfs2_info))) > return -EFAULT; > > return ocfs2_info_handle(inode, &info, 0); > + } > case FITRIM: > { > struct super_block *sb = inode->i_sb;
On Mon, Apr 17, 2023 at 1:56 PM Arnd Bergmann <arnd@kernel.org> wrote: > On 32-bit architectures with KASAN_STACK enabled, the total stack usage > of the ocfs2_ioctl function grows beyond the warning limit: > > fs/ocfs2/ioctl.c: In function 'ocfs2_ioctl': > fs/ocfs2/ioctl.c:934:1: error: the frame size of 1448 bytes is larger than 1400 bytes [-Werror=frame-larger-than=] > > Move each of the variables into a basic block, and mark ocfs2_info_handle() > as noinline_for_stack, in order to have the variable share stack slots. Thanks for this, Reviewed-by: Mark Fasheh <mark@fasheh.com> --Mark
Andrew picked ocfs2 patches into -mm tree before. Thanks, Joseph On 4/18/23 5:17 PM, Christian Brauner wrote: > > On Mon, 17 Apr 2023 22:56:24 +0200, Arnd Bergmann wrote: >> On 32-bit architectures with KASAN_STACK enabled, the total stack usage >> of the ocfs2_ioctl function grows beyond the warning limit: >> >> fs/ocfs2/ioctl.c: In function 'ocfs2_ioctl': >> fs/ocfs2/ioctl.c:934:1: error: the frame size of 1448 bytes is larger than 1400 bytes [-Werror=frame-larger-than=] >> >> Move each of the variables into a basic block, and mark ocfs2_info_handle() >> as noinline_for_stack, in order to have the variable share stack slots. >> >> [...] > > Going by git log, ocfs2 patches don't go through a separate tree. > So unless there are objections I'm taking this through fs.misc, > > tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git > branch: fs.misc > [1/1] ocfs2: reduce ioctl stack usage > commit: 85ef56bc2d65215f43ceb7377ca14a779468928d > > Christian
On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > Andrew picked ocfs2 patches into -mm tree before. Yup and that's fine obviously, but this belongs to fs/ and we're aiming to take fs/ stuff through the dedicated fs trees going forward. Thanks! Christian > > Thanks, > Joseph > > On 4/18/23 5:17 PM, Christian Brauner wrote: > > > > On Mon, 17 Apr 2023 22:56:24 +0200, Arnd Bergmann wrote: > >> On 32-bit architectures with KASAN_STACK enabled, the total stack usage > >> of the ocfs2_ioctl function grows beyond the warning limit: > >> > >> fs/ocfs2/ioctl.c: In function 'ocfs2_ioctl': > >> fs/ocfs2/ioctl.c:934:1: error: the frame size of 1448 bytes is larger than 1400 bytes [-Werror=frame-larger-than=] > >> > >> Move each of the variables into a basic block, and mark ocfs2_info_handle() > >> as noinline_for_stack, in order to have the variable share stack slots. > >> > >> [...] > > > > Going by git log, ocfs2 patches don't go through a separate tree. > > So unless there are objections I'm taking this through fs.misc, > > > > tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git > > branch: fs.misc > > [1/1] ocfs2: reduce ioctl stack usage > > commit: 85ef56bc2d65215f43ceb7377ca14a779468928d > > > > Christian
On Tue, Apr 18, 2023 at 02:56:38PM +0200, Christian Brauner wrote: > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > > Andrew picked ocfs2 patches into -mm tree before. > > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > to take fs/ stuff through the dedicated fs trees going forward. The challenge here is that there isn't an active ocfs2 tree at the moment (git/jlbec/ocfs2.git was last updated 11 years ago), and it's not clear ocfs2 has an active maintainer, although as discussed at a recent ext4 telechat, there still are some enterprise users of it hanging on. We're starting considering changes to fs/jbd2 so we can get off of using the struct bh for jbd2 (still very early days; it's more in the early concept brain-storming stage.) And since ocfs2 also uses fs/jbd2, the fact that ocfs2 isn't under active maintenance is going to be more of a challenge moving forward. Something we should discuss in the hallway track of LSF/MM, perhaps... - Ted
On 4/18/23 8:56 PM, Christian Brauner wrote: > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: >> Andrew picked ocfs2 patches into -mm tree before. > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > to take fs/ stuff through the dedicated fs trees going forward. Either is fine for me. Hi Andrew, what's your opinion? Thanks, Joseph
On Wed, 19 Apr 2023 10:00:15 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote: > > > On 4/18/23 8:56 PM, Christian Brauner wrote: > > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > >> Andrew picked ocfs2 patches into -mm tree before. > > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > > to take fs/ stuff through the dedicated fs trees going forward. > > Either is fine for me. > Hi Andrew, what's your opinion? I've been wrangling ocfs2 for over a decade and this is the first I've heard of this proposal. Who is "we", above? What was their reasoning? Who will be responsible for ocfs2 patches? What will be their workflow and review and test processes? Overall, what benefit does this proposal offer the ocfs2 project?
On Wed, Apr 19, 2023 at 02:21:59PM -0700, Andrew Morton wrote: > On Wed, 19 Apr 2023 10:00:15 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote: > > > > > > > On 4/18/23 8:56 PM, Christian Brauner wrote: > > > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > > >> Andrew picked ocfs2 patches into -mm tree before. > > > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > > > to take fs/ stuff through the dedicated fs trees going forward. > > > > Either is fine for me. > > Hi Andrew, what's your opinion? > > I've been wrangling ocfs2 for over a decade and this is the first I've > heard of this proposal. > > Who is "we", above? What was their reasoning? > > Who will be responsible for ocfs2 patches? What will be their workflow > and review and test processes? > > Overall, what benefit does this proposal offer the ocfs2 project? I think I might not have communicated as clearly as I should have. Simply because I naively assumed that this is unproblematic. By "we" I mean people responsible for "fs/" which now happens to also include me. So the goal of this is for patches falling under fs/ to get picked up more quickly and broadly and share the maintenance burden. Since ocfs2 falls under fs/ it felt pretty straightforward that it should go via one of the fs/ trees and thus I picked it up and didn't bat an eye that it might somehow bother you. For us as in "fs/" it's nicer because it means if we do fs wide changes we'll reduce chances of merge conflicts.
On Thu, Apr 20, 2023 at 2:34 AM Christian Brauner <brauner@kernel.org> wrote: > I think I might not have communicated as clearly as I should have. > Simply because I naively assumed that this is unproblematic. > > By "we" I mean people responsible for "fs/" which now happens to also > include me. So the goal of this is for patches falling under fs/ to get > picked up more quickly and broadly and share the maintenance burden. Did you get buy-in from other folks in 'fs/'? What other projects are you carrying? Granted I'm a bit out of the loop these days but this is the first I'm hearing of this. Andrew has a well oiled machine going, so if he's still ok carrying the patches then that's where I'd like them until such time that you can provide a tangible benefit. Thanks, --Mark
On Tue, Apr 18, 2023 at 02:56:38PM +0200, Christian Brauner wrote: > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > > Andrew picked ocfs2 patches into -mm tree before. > > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > to take fs/ stuff through the dedicated fs trees going forward. Er... Assuming that there *is* an active fs tree for filesystem in question. Do you really want dedicated e.g. affs, adfs, etc. git trees - one for each filesystem in there?
On Thu, Apr 20, 2023 at 09:48:01PM +0100, Al Viro wrote: > On Tue, Apr 18, 2023 at 02:56:38PM +0200, Christian Brauner wrote: > > On Tue, Apr 18, 2023 at 05:37:06PM +0800, Joseph Qi wrote: > > > Andrew picked ocfs2 patches into -mm tree before. > > > > Yup and that's fine obviously, but this belongs to fs/ and we're aiming > > to take fs/ stuff through the dedicated fs trees going forward. > > Er... Assuming that there *is* an active fs tree for filesystem > in question. Do you really want dedicated e.g. affs, adfs, etc. > git trees - one for each filesystem in there? No, that's not meant or want. What I meant is that when a filesystem doesn't have a dedicated tree (and/or active maintainers) mentioned in the maintainer's file then we pick up those patches just like we already do today and have done.
On Thu, Apr 20, 2023 at 10:01:12AM -0700, Mark Fasheh wrote: > On Thu, Apr 20, 2023 at 2:34 AM Christian Brauner <brauner@kernel.org> wrote: > > I think I might not have communicated as clearly as I should have. > > Simply because I naively assumed that this is unproblematic. > > > > By "we" I mean people responsible for "fs/" which now happens to also > > include me. So the goal of this is for patches falling under fs/ to get > > picked up more quickly and broadly and share the maintenance burden. > > Did you get buy-in from other folks in 'fs/'? What other projects are > you carrying? Granted I'm a bit out of the loop these days but this is > the first I'm hearing of this. > > Andrew has a well oiled machine going, so if he's still ok carrying > the patches then that's where I'd like them until such time that you > can provide a tangible benefit. A patch is sent for something that falls under the fs/ directory. In this case fs/ocfs2/. The maintainer's of fs/ocfs2/ provide their acks. A maintainer - In this case my sorry ass - of fs/ looks into the maintainer's file to make sure that someone will pick up those patches by looking for a tree entry under the respective fs/ocfs2/ entry. There is no tree entry. So the patch is picked up by a respective maintainer of fs/ to ensure that fixes land in mainline. So, if you have a tree that you think fs/ocfs2/ belongs to then please send a patch to add the respective tree into the maintainer's file. This is especially true when fs/ stuff surprisingly goes via mm/. I don't want to have to guess what tree you're going through even if it's been going on for a long time. There are no bad intentions here but please clarify the ocfs2 entry in the maintainer's file.
diff --git a/fs/ocfs2/ioctl.c b/fs/ocfs2/ioctl.c index 811a6ea374bb..b1550ba73f96 100644 --- a/fs/ocfs2/ioctl.c +++ b/fs/ocfs2/ioctl.c @@ -803,8 +803,8 @@ static int ocfs2_get_request_ptr(struct ocfs2_info *info, int idx, * a better backward&forward compatibility, since a small piece of * request will be less likely to be broken if disk layout get changed. */ -static int ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, - int compat_flag) +static noinline_for_stack int +ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, int compat_flag) { int i, status = 0; u64 req_addr; @@ -840,27 +840,26 @@ static int ocfs2_info_handle(struct inode *inode, struct ocfs2_info *info, long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { struct inode *inode = file_inode(filp); - int new_clusters; - int status; - struct ocfs2_space_resv sr; - struct ocfs2_new_group_input input; - struct reflink_arguments args; - const char __user *old_path; - const char __user *new_path; - bool preserve; - struct ocfs2_info info; void __user *argp = (void __user *)arg; + int status; switch (cmd) { case OCFS2_IOC_RESVSP: case OCFS2_IOC_RESVSP64: case OCFS2_IOC_UNRESVSP: case OCFS2_IOC_UNRESVSP64: + { + struct ocfs2_space_resv sr; + if (copy_from_user(&sr, (int __user *) arg, sizeof(sr))) return -EFAULT; return ocfs2_change_file_space(filp, cmd, &sr); + } case OCFS2_IOC_GROUP_EXTEND: + { + int new_clusters; + if (!capable(CAP_SYS_RESOURCE)) return -EPERM; @@ -873,8 +872,12 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) status = ocfs2_group_extend(inode, new_clusters); mnt_drop_write_file(filp); return status; + } case OCFS2_IOC_GROUP_ADD: case OCFS2_IOC_GROUP_ADD64: + { + struct ocfs2_new_group_input input; + if (!capable(CAP_SYS_RESOURCE)) return -EPERM; @@ -887,7 +890,14 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) status = ocfs2_group_add(inode, &input); mnt_drop_write_file(filp); return status; + } case OCFS2_IOC_REFLINK: + { + struct reflink_arguments args; + const char __user *old_path; + const char __user *new_path; + bool preserve; + if (copy_from_user(&args, argp, sizeof(args))) return -EFAULT; old_path = (const char __user *)(unsigned long)args.old_path; @@ -895,11 +905,16 @@ long ocfs2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) preserve = (args.preserve != 0); return ocfs2_reflink_ioctl(inode, old_path, new_path, preserve); + } case OCFS2_IOC_INFO: + { + struct ocfs2_info info; + if (copy_from_user(&info, argp, sizeof(struct ocfs2_info))) return -EFAULT; return ocfs2_info_handle(inode, &info, 0); + } case FITRIM: { struct super_block *sb = inode->i_sb;