Re: [PATCH] Btrfs: do not overwrite error return value in the balance ioctl

[David Sterba <dsterba@suse.cz>]

On Mon, Dec 17, 2018 at 09:24:43AM +0000, Filipe Manana wrote:
> On Mon, Dec 17, 2018 at 8:25 AM Anand Jain <anand.jain@oracle.com> wrote:
> >
> >
> >
> > On 12/15/2018 03:45 AM, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > If the call to btrfs_balance() failed we would overwrite the error
> > > returned to user space with -EFAULT if the call to copy_to_user() failed
> > > as well. Fix that by calling copy_to_user() only if btrfs_balance()
> > > returned success.
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > >   fs/btrfs/ioctl.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > index abe45fd97ab5..4ad7288f77d0 100644
> > > --- a/fs/btrfs/ioctl.c
> > > +++ b/fs/btrfs/ioctl.c
> > > @@ -4707,7 +4707,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
> > >       ret = btrfs_balance(fs_info, bctl, bargs);
> > >       bctl = NULL;
> > >
> > > -     if (arg) {
> > > +     if (ret == 0 && arg) {
> > >               if (copy_to_user(arg, bargs, sizeof(*bargs)))
> > >                       ret = -EFAULT;
> > >       }
> > >
> >
> > This changes the BTRFS_IOC_BALANCE_V2 semantics.
> >
> > Originally, if BTRFS_IOC_BALANCE_V2 is failed/canceled we do update
> > bargs [1] and copy to user land, but after this patch we copy only
> > if the ioctl is successful. I doubt if David is OK with that.
> 
> I thought about that. But that logic, if it's really in use by someone
> (something) out there then they must be treating -EFAULT differently
> from any other error.
> Because if copy_to_user() fails it means it didn't copy all bytes it
> was so supposed to (its return value is number of bytes not copied),
> so the arguments structure ends up in an inconsistent state.

The balance ioctl as used in btrfs-progs expects valid contents of the
bargs even if the ioctl returns negative value, with errno set to
ECANCELED:

https://github.com/kdave/btrfs-progs/blob/master/cmds-balance.c#L458

So the condition would need to take that into account

	if ((ret == 0 || ret == -ECANCELED) && arg)

I haven't found any other error that would need to be whitelisted here.