[PATCH] ocfs2: reduce ioctl stack usage

[PATCH] ocfs2: reduce ioctl stack usage

[Arnd Bergmann]

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>
---
 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;
-- 
2.39.2

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Joseph Qi]

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;

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Mark Fasheh]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Joseph Qi]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Christian Brauner]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Theodore Ts'o]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Joseph Qi]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Andrew Morton]

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?

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Christian Brauner]

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.

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Mark Fasheh]

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

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Al Viro]

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?

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Christian Brauner]

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.

Re: [PATCH] ocfs2: reduce ioctl stack usage

[Christian Brauner]

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.