| Message ID | 78bf853ef63f2dbd62587bb02bb723fbaa77c198.1600070418.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix wrong address when faulting in pages in the search ioctl | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 9/14/20 4:01 AM, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When faulting in the pages for the user supplied buffer for the search > ioctl, we are passing only the base address of the buffer to the function > fault_in_pages_writeable(). This means that after the first iteration of > the while loop that searches for leaves, when we have a non-zero offset, > stored in 'sk_offset', we try to fault in a wrong page range. > > So fix this by adding the offset in 'sk_offset' to the base address of the > user supplied buffer when calling fault_in_pages_writeable(). > > Several users have reported that the applications compsize and bees have > started to operate incorrectly since commit a48b73eca4ceb9 ("btrfs: fix > potential deadlock in the search ioctl") was added to stable trees, and > these applications make heavy use of the search ioctls. This fixes their > issues. > > Link: https://lore.kernel.org/linux-btrfs/632b888d-a3c3-b085-cdf5-f9bb61017d92@lechevalier.se/ > Link: https://github.com/kilobyte/compsize/issues/34 > Tested-by: A L <mail@lechevalier.se> > Fixes: a48b73eca4ceb9 ("btrfs: fix potential deadlock in the search ioctl") > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/ioctl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 5f06aeb71823..b91444e810a5 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2194,7 +2194,8 @@ static noinline int search_ioctl(struct inode *inode, > key.offset = sk->min_offset; > > while (1) { > - ret = fault_in_pages_writeable(ubuf, *buf_size - sk_offset); > + ret = fault_in_pages_writeable(ubuf + sk_offset, > + *buf_size - sk_offset); > if (ret) > break; > > Eesh, can we get an xfstest for this? Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
On Mon, Sep 14, 2020 at 4:23 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On 9/14/20 4:01 AM, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > When faulting in the pages for the user supplied buffer for the search > > ioctl, we are passing only the base address of the buffer to the function > > fault_in_pages_writeable(). This means that after the first iteration of > > the while loop that searches for leaves, when we have a non-zero offset, > > stored in 'sk_offset', we try to fault in a wrong page range. > > > > So fix this by adding the offset in 'sk_offset' to the base address of the > > user supplied buffer when calling fault_in_pages_writeable(). > > > > Several users have reported that the applications compsize and bees have > > started to operate incorrectly since commit a48b73eca4ceb9 ("btrfs: fix > > potential deadlock in the search ioctl") was added to stable trees, and > > these applications make heavy use of the search ioctls. This fixes their > > issues. > > > > Link: https://lore.kernel.org/linux-btrfs/632b888d-a3c3-b085-cdf5-f9bb61017d92@lechevalier.se/ > > Link: https://github.com/kilobyte/compsize/issues/34 > > Tested-by: A L <mail@lechevalier.se> > > Fixes: a48b73eca4ceb9 ("btrfs: fix potential deadlock in the search ioctl") > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/ioctl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 5f06aeb71823..b91444e810a5 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -2194,7 +2194,8 @@ static noinline int search_ioctl(struct inode *inode, > > key.offset = sk->min_offset; > > > > while (1) { > > - ret = fault_in_pages_writeable(ubuf, *buf_size - sk_offset); > > + ret = fault_in_pages_writeable(ubuf + sk_offset, > > + *buf_size - sk_offset); > > if (ret) > > break; > > > > > > Eesh, can we get an xfstest for this? I would have written one if it were simple to trigger. I don't think it's easy to trigger it deterministically or close enough to deterministic. How do you guarantee that a call to the ioctl will cause the pages to fault in at any iteration of the loop? That the pages aren't already faulted in, and then if you need to fault them in, that happens on an iteration other than the first one? Thanks. > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > Thanks, > > Josef
On 9/14/20 11:47 AM, Filipe Manana wrote: > On Mon, Sep 14, 2020 at 4:23 PM Josef Bacik <josef@toxicpanda.com> wrote: >> >> On 9/14/20 4:01 AM, fdmanana@kernel.org wrote: >>> From: Filipe Manana <fdmanana@suse.com> >>> >>> When faulting in the pages for the user supplied buffer for the search >>> ioctl, we are passing only the base address of the buffer to the function >>> fault_in_pages_writeable(). This means that after the first iteration of >>> the while loop that searches for leaves, when we have a non-zero offset, >>> stored in 'sk_offset', we try to fault in a wrong page range. >>> >>> So fix this by adding the offset in 'sk_offset' to the base address of the >>> user supplied buffer when calling fault_in_pages_writeable(). >>> >>> Several users have reported that the applications compsize and bees have >>> started to operate incorrectly since commit a48b73eca4ceb9 ("btrfs: fix >>> potential deadlock in the search ioctl") was added to stable trees, and >>> these applications make heavy use of the search ioctls. This fixes their >>> issues. >>> >>> Link: https://lore.kernel.org/linux-btrfs/632b888d-a3c3-b085-cdf5-f9bb61017d92@lechevalier.se/ >>> Link: https://github.com/kilobyte/compsize/issues/34 >>> Tested-by: A L <mail@lechevalier.se> >>> Fixes: a48b73eca4ceb9 ("btrfs: fix potential deadlock in the search ioctl") >>> Signed-off-by: Filipe Manana <fdmanana@suse.com> >>> --- >>> fs/btrfs/ioctl.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >>> index 5f06aeb71823..b91444e810a5 100644 >>> --- a/fs/btrfs/ioctl.c >>> +++ b/fs/btrfs/ioctl.c >>> @@ -2194,7 +2194,8 @@ static noinline int search_ioctl(struct inode *inode, >>> key.offset = sk->min_offset; >>> >>> while (1) { >>> - ret = fault_in_pages_writeable(ubuf, *buf_size - sk_offset); >>> + ret = fault_in_pages_writeable(ubuf + sk_offset, >>> + *buf_size - sk_offset); >>> if (ret) >>> break; >>> >>> >> >> Eesh, can we get an xfstest for this? > > I would have written one if it were simple to trigger. > I don't think it's easy to trigger it deterministically or close > enough to deterministic. > > How do you guarantee that a call to the ioctl will cause the pages to > fault in at any iteration of the loop? > That the pages aren't already faulted in, and then if you need to > fault them in, that happens on an iteration other than the first one? > Yeah I was thinking about it after I wrote it. I guess mmap a few pages worth, and stick the first entry at the end of the first page? A stress test is probably in order for this stuff so we don't have this sort of problem again. Thanks, Josef
On Mon, Sep 14, 2020 at 09:01:04AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When faulting in the pages for the user supplied buffer for the search > ioctl, we are passing only the base address of the buffer to the function > fault_in_pages_writeable(). This means that after the first iteration of > the while loop that searches for leaves, when we have a non-zero offset, > stored in 'sk_offset', we try to fault in a wrong page range. > > So fix this by adding the offset in 'sk_offset' to the base address of the > user supplied buffer when calling fault_in_pages_writeable(). > > Several users have reported that the applications compsize and bees have > started to operate incorrectly since commit a48b73eca4ceb9 ("btrfs: fix > potential deadlock in the search ioctl") was added to stable trees, and > these applications make heavy use of the search ioctls. This fixes their > issues. > > Link: https://lore.kernel.org/linux-btrfs/632b888d-a3c3-b085-cdf5-f9bb61017d92@lechevalier.se/ > Link: https://github.com/kilobyte/compsize/issues/34 > Tested-by: A L <mail@lechevalier.se> > Fixes: a48b73eca4ceb9 ("btrfs: fix potential deadlock in the search ioctl") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Thanks, it's on the way to mainline and stable trees.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 5f06aeb71823..b91444e810a5 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2194,7 +2194,8 @@ static noinline int search_ioctl(struct inode *inode, key.offset = sk->min_offset; while (1) { - ret = fault_in_pages_writeable(ubuf, *buf_size - sk_offset); + ret = fault_in_pages_writeable(ubuf + sk_offset, + *buf_size - sk_offset); if (ret) break;