Btrfs: Fix memory corruption by ulist_add_merge() on 32bit arch

Commit Message

Takashi Iwai July 28, 2014, 8:57 a.m. UTC

We've got bug reports that btrfs crashes when quota is enabled on
32bit kernel, typically with the Oops like below:
 BUG: unable to handle kernel NULL pointer dereference at 00000004
 IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
 *pde = 00000000
 Oops: 0000 [#1] SMP
 CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
 Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
 task: f1478130 ti: f147c000 task.ti: f147c000
 EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
 EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
 EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
 ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
 CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
 Stack:
  00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
  00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
  00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
 Call Trace:
  [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
  [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
  [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
  [<c025e38b>] process_one_work+0x11b/0x390
  [<c025eea1>] worker_thread+0x101/0x340
  [<c026432b>] kthread+0x9b/0xb0
  [<c0712a71>] ret_from_kernel_thread+0x21/0x30
  [<c0264290>] kthread_create_on_node+0x110/0x110

This indicates a NULL corruption in prefs_delayed list.  The further
investigation and bisection pointed that the call of ulist_add_merge()
results in the corruption.

ulist_add_merge() takes u64 as aux and writes a 64bit value into
old_aux.  The callers of this function in backref.c, however, pass a
pointer of a pointer to old_aux.  That is, the function overwrites
64bit value on 32bit pointer.  This caused a NULL in the adjacent
variable, in this case, prefs_delayed.

Here is a quick attempt to band-aid over this: a new function,
ulist_add_merge_ptr() is introduced to pass/store properly a pointer
value instead of u64.  There are still ugly void ** cast remaining
in the callers because void ** cannot be taken implicitly.  But, it's
safer than explicit cast to u64, anyway.

Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=887046
Cc: <[e-mail]> [v3.11+]
Signed-off-by: Takashi Iwai <[e-mail]>
---

Alternatively, we can change the argument of aux and old_aux to a
pointer from u64, as backref.c is the only user of ulist_add_merge()
function.  I'll cook up another patch if it's the preferred way.

 fs/btrfs/backref.c | 11 +++++------
 fs/btrfs/ulist.h   | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

Comments

Wang Shilong July 28, 2014, 9:38 a.m. UTC | #1

Hi Takashi,

This seems like a promising fix, i just have a little question:

ulist_add() logic is we firstly cast a pointer to u64(see ptr_to_u64()), 
and then
cast u64 to pointer back(u64_to_ptr()). So normally, arg u64 is actually 
a pointer.

If the below overflow happens, that means casting can change original value?

Am i missing something here?

Thanks,
Wang
On 07/28/2014 04:57 PM, Takashi Iwai wrote:
> We've got bug reports that btrfs crashes when quota is enabled on
> 32bit kernel, typically with the Oops like below:
>   BUG: unable to handle kernel NULL pointer dereference at 00000004
>   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>   *pde = 00000000
>   Oops: 0000 [#1] SMP
>   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>   task: f1478130 ti: f147c000 task.ti: f147c000
>   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>   Stack:
>    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>   Call Trace:
>    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>    [<c025e38b>] process_one_work+0x11b/0x390
>    [<c025eea1>] worker_thread+0x101/0x340
>    [<c026432b>] kthread+0x9b/0xb0
>    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>    [<c0264290>] kthread_create_on_node+0x110/0x110
>
> This indicates a NULL corruption in prefs_delayed list.  The further
> investigation and bisection pointed that the call of ulist_add_merge()
> results in the corruption.
>
> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> old_aux.  The callers of this function in backref.c, however, pass a
> pointer of a pointer to old_aux.  That is, the function overwrites
> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> variable, in this case, prefs_delayed.
>
> Here is a quick attempt to band-aid over this: a new function,
> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> value instead of u64.  There are still ugly void ** cast remaining
> in the callers because void ** cannot be taken implicitly.  But, it's
> safer than explicit cast to u64, anyway.
>
> Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=887046
> Cc: <[e-mail]> [v3.11+]
> Signed-off-by: Takashi Iwai <[e-mail]>
> ---
>
> Alternatively, we can change the argument of aux and old_aux to a
> pointer from u64, as backref.c is the only user of ulist_add_merge()
> function.  I'll cook up another patch if it's the preferred way.
>
>   fs/btrfs/backref.c | 11 +++++------
>   fs/btrfs/ulist.h   | 15 +++++++++++++++
>   2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index e25564bfcb46..d7a24620a963 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -276,9 +276,8 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
>   			}
>   			if (ret > 0)
>   				goto next;
> -			ret = ulist_add_merge(parents, eb->start,
> -					      (uintptr_t)eie,
> -					      (u64 *)&old, GFP_NOFS);
> +			ret = ulist_add_merge_ptr(parents, eb->start,
> +						  eie, (void **)&old, GFP_NOFS);
>   			if (ret < 0)
>   				break;
>   			if (!ret && extent_item_pos) {
> @@ -1008,9 +1007,9 @@ again:
>   					goto out;
>   				ref->inode_list = eie;
>   			}
> -			ret = ulist_add_merge(refs, ref->parent,
> -					      (uintptr_t)ref->inode_list,
> -					      (u64 *)&eie, GFP_NOFS);
> +			ret = ulist_add_merge_ptr(refs, ref->parent,
> +						  ref->inode_list,
> +						  (void **)&eie, GFP_NOFS);
>   			if (ret < 0)
>   				goto out;
>   			if (!ret && extent_item_pos) {
> diff --git a/fs/btrfs/ulist.h b/fs/btrfs/ulist.h
> index 7f78cbf5cf41..695fc2bac680 100644
> --- a/fs/btrfs/ulist.h
> +++ b/fs/btrfs/ulist.h
> @@ -57,6 +57,21 @@ void ulist_free(struct ulist *ulist);
>   int ulist_add(struct ulist *ulist, u64 val, u64 aux, gfp_t gfp_mask);
>   int ulist_add_merge(struct ulist *ulist, u64 val, u64 aux,
>   		    u64 *old_aux, gfp_t gfp_mask);
> +
> +/* just like ulist_add_merge() but take a pointer for the aux data */
> +static inline int ulist_add_merge_ptr(struct ulist *ulist, u64 val, void *aux,
> +				      void **old_aux, gfp_t gfp_mask)
> +{
> +#if BITS_PER_LONG == 32
> +	u64 old64 = (uintptr_t)*old_aux;
> +	int ret = ulist_add_merge(ulist, val, (uintptr_t)aux, &old64, gfp_mask);
> +	*old_aux = (void *)((uintptr_t)old64);
> +	return ret;
> +#else
> +	return ulist_add_merge(ulist, val, (u64)aux, (u64 *)old_aux, gfp_mask)
> +#endif
> +}
> +
>   struct ulist_node *ulist_next(struct ulist *ulist,
>   			      struct ulist_iterator *uiter);
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 28, 2014, 9:49 a.m. UTC | #2

At Mon, 28 Jul 2014 17:38:52 +0800,
Wang Shilong wrote:
> 
> Hi Takashi,
> 
> This seems like a promising fix, i just have a little question:
> 
> ulist_add() logic is we firstly cast a pointer to u64(see ptr_to_u64()), 
> and then
> cast u64 to pointer back(u64_to_ptr()). So normally, arg u64 is actually 
> a pointer.
> 
> If the below overflow happens, that means casting can change original value?
> 
> Am i missing something here?

Casting between u64 and pointer instantaneously is OK.  The problem
is, however, old_aux taking a u64 pointer.  It writes a 64bit value on
the given address, while the caller passed the address for a 32bit
data.


Takashi

> 
> Thanks,
> Wang
> On 07/28/2014 04:57 PM, Takashi Iwai wrote:
> > We've got bug reports that btrfs crashes when quota is enabled on
> > 32bit kernel, typically with the Oops like below:
> >   BUG: unable to handle kernel NULL pointer dereference at 00000004
> >   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >   *pde = 00000000
> >   Oops: 0000 [#1] SMP
> >   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >   task: f1478130 ti: f147c000 task.ti: f147c000
> >   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >   Stack:
> >    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >   Call Trace:
> >    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >    [<c025e38b>] process_one_work+0x11b/0x390
> >    [<c025eea1>] worker_thread+0x101/0x340
> >    [<c026432b>] kthread+0x9b/0xb0
> >    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >    [<c0264290>] kthread_create_on_node+0x110/0x110
> >
> > This indicates a NULL corruption in prefs_delayed list.  The further
> > investigation and bisection pointed that the call of ulist_add_merge()
> > results in the corruption.
> >
> > ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > old_aux.  The callers of this function in backref.c, however, pass a
> > pointer of a pointer to old_aux.  That is, the function overwrites
> > 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > variable, in this case, prefs_delayed.
> >
> > Here is a quick attempt to band-aid over this: a new function,
> > ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > value instead of u64.  There are still ugly void ** cast remaining
> > in the callers because void ** cannot be taken implicitly.  But, it's
> > safer than explicit cast to u64, anyway.
> >
> > Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=887046
> > Cc: <[e-mail]> [v3.11+]
> > Signed-off-by: Takashi Iwai <[e-mail]>
> > ---
> >
> > Alternatively, we can change the argument of aux and old_aux to a
> > pointer from u64, as backref.c is the only user of ulist_add_merge()
> > function.  I'll cook up another patch if it's the preferred way.
> >
> >   fs/btrfs/backref.c | 11 +++++------
> >   fs/btrfs/ulist.h   | 15 +++++++++++++++
> >   2 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index e25564bfcb46..d7a24620a963 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -276,9 +276,8 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
> >   			}
> >   			if (ret > 0)
> >   				goto next;
> > -			ret = ulist_add_merge(parents, eb->start,
> > -					      (uintptr_t)eie,
> > -					      (u64 *)&old, GFP_NOFS);
> > +			ret = ulist_add_merge_ptr(parents, eb->start,
> > +						  eie, (void **)&old, GFP_NOFS);
> >   			if (ret < 0)
> >   				break;
> >   			if (!ret && extent_item_pos) {
> > @@ -1008,9 +1007,9 @@ again:
> >   					goto out;
> >   				ref->inode_list = eie;
> >   			}
> > -			ret = ulist_add_merge(refs, ref->parent,
> > -					      (uintptr_t)ref->inode_list,
> > -					      (u64 *)&eie, GFP_NOFS);
> > +			ret = ulist_add_merge_ptr(refs, ref->parent,
> > +						  ref->inode_list,
> > +						  (void **)&eie, GFP_NOFS);
> >   			if (ret < 0)
> >   				goto out;
> >   			if (!ret && extent_item_pos) {
> > diff --git a/fs/btrfs/ulist.h b/fs/btrfs/ulist.h
> > index 7f78cbf5cf41..695fc2bac680 100644
> > --- a/fs/btrfs/ulist.h
> > +++ b/fs/btrfs/ulist.h
> > @@ -57,6 +57,21 @@ void ulist_free(struct ulist *ulist);
> >   int ulist_add(struct ulist *ulist, u64 val, u64 aux, gfp_t gfp_mask);
> >   int ulist_add_merge(struct ulist *ulist, u64 val, u64 aux,
> >   		    u64 *old_aux, gfp_t gfp_mask);
> > +
> > +/* just like ulist_add_merge() but take a pointer for the aux data */
> > +static inline int ulist_add_merge_ptr(struct ulist *ulist, u64 val, void *aux,
> > +				      void **old_aux, gfp_t gfp_mask)
> > +{
> > +#if BITS_PER_LONG == 32
> > +	u64 old64 = (uintptr_t)*old_aux;
> > +	int ret = ulist_add_merge(ulist, val, (uintptr_t)aux, &old64, gfp_mask);
> > +	*old_aux = (void *)((uintptr_t)old64);
> > +	return ret;
> > +#else
> > +	return ulist_add_merge(ulist, val, (u64)aux, (u64 *)old_aux, gfp_mask)
> > +#endif
> > +}
> > +
> >   struct ulist_node *ulist_next(struct ulist *ulist,
> >   			      struct ulist_iterator *uiter);
> >   
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josef Bacik July 28, 2014, 1:16 p.m. UTC | #3

On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> We've got bug reports that btrfs crashes when quota is enabled on
> 32bit kernel, typically with the Oops like below:
>   BUG: unable to handle kernel NULL pointer dereference at 00000004
>   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>   *pde = 00000000
>   Oops: 0000 [#1] SMP
>   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>   task: f1478130 ti: f147c000 task.ti: f147c000
>   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>   Stack:
>    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>   Call Trace:
>    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>    [<c025e38b>] process_one_work+0x11b/0x390
>    [<c025eea1>] worker_thread+0x101/0x340
>    [<c026432b>] kthread+0x9b/0xb0
>    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>    [<c0264290>] kthread_create_on_node+0x110/0x110
>
> This indicates a NULL corruption in prefs_delayed list.  The further
> investigation and bisection pointed that the call of ulist_add_merge()
> results in the corruption.
>
> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> old_aux.  The callers of this function in backref.c, however, pass a
> pointer of a pointer to old_aux.  That is, the function overwrites
> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> variable, in this case, prefs_delayed.
>
> Here is a quick attempt to band-aid over this: a new function,
> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> value instead of u64.  There are still ugly void ** cast remaining
> in the callers because void ** cannot be taken implicitly.  But, it's
> safer than explicit cast to u64, anyway.
>
> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> Cc: <[e-mail]> [v3.11+]
> Signed-off-by: Takashi Iwai <[e-mail]>
> ---
>
> Alternatively, we can change the argument of aux and old_aux to a
> pointer from u64, as backref.c is the only user of ulist_add_merge()
> function.  I'll cook up another patch if it's the preferred way.
>

Yeah lets just use a pointer and see how that works out.  Thanks,

Josef

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 28, 2014, 1:48 p.m. UTC | #4

At Mon, 28 Jul 2014 09:16:48 -0400,
Josef Bacik wrote:
> 
> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> > We've got bug reports that btrfs crashes when quota is enabled on
> > 32bit kernel, typically with the Oops like below:
> >   BUG: unable to handle kernel NULL pointer dereference at 00000004
> >   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >   *pde = 00000000
> >   Oops: 0000 [#1] SMP
> >   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >   task: f1478130 ti: f147c000 task.ti: f147c000
> >   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >   Stack:
> >    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >   Call Trace:
> >    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >    [<c025e38b>] process_one_work+0x11b/0x390
> >    [<c025eea1>] worker_thread+0x101/0x340
> >    [<c026432b>] kthread+0x9b/0xb0
> >    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >    [<c0264290>] kthread_create_on_node+0x110/0x110
> >
> > This indicates a NULL corruption in prefs_delayed list.  The further
> > investigation and bisection pointed that the call of ulist_add_merge()
> > results in the corruption.
> >
> > ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > old_aux.  The callers of this function in backref.c, however, pass a
> > pointer of a pointer to old_aux.  That is, the function overwrites
> > 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > variable, in this case, prefs_delayed.
> >
> > Here is a quick attempt to band-aid over this: a new function,
> > ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > value instead of u64.  There are still ugly void ** cast remaining
> > in the callers because void ** cannot be taken implicitly.  But, it's
> > safer than explicit cast to u64, anyway.
> >
> > Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> > Cc: <[e-mail]> [v3.11+]
> > Signed-off-by: Takashi Iwai <[e-mail]>
> > ---
> >
> > Alternatively, we can change the argument of aux and old_aux to a
> > pointer from u64, as backref.c is the only user of ulist_add_merge()
> > function.  I'll cook up another patch if it's the preferred way.
> >
> 
> Yeah lets just use a pointer and see how that works out.  Thanks,

Oops, I forgot that ulist_add() takes aux as u64 and it calling
ulist_add_merge() internally.  So, we can't change the type blindly
there, unfortunately.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 28, 2014, 2:01 p.m. UTC | #5

At Mon, 28 Jul 2014 15:48:41 +0200,
Takashi Iwai wrote:
> 
> At Mon, 28 Jul 2014 09:16:48 -0400,
> Josef Bacik wrote:
> > 
> > On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> > > We've got bug reports that btrfs crashes when quota is enabled on
> > > 32bit kernel, typically with the Oops like below:
> > >   BUG: unable to handle kernel NULL pointer dereference at 00000004
> > >   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> > >   *pde = 00000000
> > >   Oops: 0000 [#1] SMP
> > >   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> > >   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> > >   task: f1478130 ti: f147c000 task.ti: f147c000
> > >   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> > >   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> > >   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> > >   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> > >    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> > >   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> > >   Stack:
> > >    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> > >    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> > >    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> > >   Call Trace:
> > >    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> > >    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> > >    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> > >    [<c025e38b>] process_one_work+0x11b/0x390
> > >    [<c025eea1>] worker_thread+0x101/0x340
> > >    [<c026432b>] kthread+0x9b/0xb0
> > >    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> > >    [<c0264290>] kthread_create_on_node+0x110/0x110
> > >
> > > This indicates a NULL corruption in prefs_delayed list.  The further
> > > investigation and bisection pointed that the call of ulist_add_merge()
> > > results in the corruption.
> > >
> > > ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > > old_aux.  The callers of this function in backref.c, however, pass a
> > > pointer of a pointer to old_aux.  That is, the function overwrites
> > > 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > > variable, in this case, prefs_delayed.
> > >
> > > Here is a quick attempt to band-aid over this: a new function,
> > > ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > > value instead of u64.  There are still ugly void ** cast remaining
> > > in the callers because void ** cannot be taken implicitly.  But, it's
> > > safer than explicit cast to u64, anyway.
> > >
> > > Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> > > Cc: <[e-mail]> [v3.11+]
> > > Signed-off-by: Takashi Iwai <[e-mail]>
> > > ---
> > >
> > > Alternatively, we can change the argument of aux and old_aux to a
> > > pointer from u64, as backref.c is the only user of ulist_add_merge()
> > > function.  I'll cook up another patch if it's the preferred way.
> > >
> > 
> > Yeah lets just use a pointer and see how that works out.  Thanks,
> 
> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> ulist_add_merge() internally.  So, we can't change the type blindly
> there, unfortunately.

Looking back at the code, it seems that all aux arguments passed to
ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
are pointers, so far, and it'd be even cleaner to replace all these
from u64 to void *.

But, such a replacement patch will become difficult for backporting to
stable kernels (the bug existed since 3.11, at least).  So IMO, we
should put a smaller fix like my previous one, let it backported to
stable kernels, and do more comprehensive replacements to pointer on
its top.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 9:57 a.m. UTC | #6

At Mon, 28 Jul 2014 16:01:55 +0200,
Takashi Iwai wrote:
> 
> At Mon, 28 Jul 2014 15:48:41 +0200,
> Takashi Iwai wrote:
> > 
> > At Mon, 28 Jul 2014 09:16:48 -0400,
> > Josef Bacik wrote:
> > > 
> > > On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> > > > We've got bug reports that btrfs crashes when quota is enabled on
> > > > 32bit kernel, typically with the Oops like below:
> > > >   BUG: unable to handle kernel NULL pointer dereference at 00000004
> > > >   IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> > > >   *pde = 00000000
> > > >   Oops: 0000 [#1] SMP
> > > >   CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> > > >   Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> > > >   task: f1478130 ti: f147c000 task.ti: f147c000
> > > >   EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> > > >   EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> > > >   EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> > > >   ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> > > >    DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> > > >   CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> > > >   Stack:
> > > >    00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> > > >    00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> > > >    00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> > > >   Call Trace:
> > > >    [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> > > >    [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> > > >    [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> > > >    [<c025e38b>] process_one_work+0x11b/0x390
> > > >    [<c025eea1>] worker_thread+0x101/0x340
> > > >    [<c026432b>] kthread+0x9b/0xb0
> > > >    [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> > > >    [<c0264290>] kthread_create_on_node+0x110/0x110
> > > >
> > > > This indicates a NULL corruption in prefs_delayed list.  The further
> > > > investigation and bisection pointed that the call of ulist_add_merge()
> > > > results in the corruption.
> > > >
> > > > ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > > > old_aux.  The callers of this function in backref.c, however, pass a
> > > > pointer of a pointer to old_aux.  That is, the function overwrites
> > > > 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > > > variable, in this case, prefs_delayed.
> > > >
> > > > Here is a quick attempt to band-aid over this: a new function,
> > > > ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > > > value instead of u64.  There are still ugly void ** cast remaining
> > > > in the callers because void ** cannot be taken implicitly.  But, it's
> > > > safer than explicit cast to u64, anyway.
> > > >
> > > > Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> > > > Cc: <[e-mail]> [v3.11+]
> > > > Signed-off-by: Takashi Iwai <[e-mail]>
> > > > ---
> > > >
> > > > Alternatively, we can change the argument of aux and old_aux to a
> > > > pointer from u64, as backref.c is the only user of ulist_add_merge()
> > > > function.  I'll cook up another patch if it's the preferred way.
> > > >
> > > 
> > > Yeah lets just use a pointer and see how that works out.  Thanks,
> > 
> > Oops, I forgot that ulist_add() takes aux as u64 and it calling
> > ulist_add_merge() internally.  So, we can't change the type blindly
> > there, unfortunately.
> 
> Looking back at the code, it seems that all aux arguments passed to
> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> are pointers, so far, and it'd be even cleaner to replace all these
> from u64 to void *.
> 
> But, such a replacement patch will become difficult for backporting to
> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> should put a smaller fix like my previous one, let it backported to
> stable kernels, and do more comprehensive replacements to pointer on
> its top.

Ping.  Could you guys take my original patch as is, or do you prefer
changing in a different way?  If so, how?


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josef Bacik July 30, 2014, 2:29 p.m. UTC | #7

On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> At Mon, 28 Jul 2014 16:01:55 +0200,
> Takashi Iwai wrote:
>>
>> At Mon, 28 Jul 2014 15:48:41 +0200,
>> Takashi Iwai wrote:
>>>
>>> At Mon, 28 Jul 2014 09:16:48 -0400,
>>> Josef Bacik wrote:
>>>>
>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
>>>>> We've got bug reports that btrfs crashes when quota is enabled on
>>>>> 32bit kernel, typically with the Oops like below:
>>>>>    BUG: unable to handle kernel NULL pointer dereference at 00000004
>>>>>    IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>    *pde = 00000000
>>>>>    Oops: 0000 [#1] SMP
>>>>>    CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>>>>>    Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>>>>>    task: f1478130 ti: f147c000 task.ti: f147c000
>>>>>    EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>>>>>    EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>    EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>>>>>    ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>>>>>     DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>>>>>    CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>>>>>    Stack:
>>>>>     00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>>>>>     00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>>>>>     00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>>>>>    Call Trace:
>>>>>     [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>>>>>     [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>>>>>     [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>>>>>     [<c025e38b>] process_one_work+0x11b/0x390
>>>>>     [<c025eea1>] worker_thread+0x101/0x340
>>>>>     [<c026432b>] kthread+0x9b/0xb0
>>>>>     [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>>>>>     [<c0264290>] kthread_create_on_node+0x110/0x110
>>>>>
>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
>>>>> investigation and bisection pointed that the call of ulist_add_merge()
>>>>> results in the corruption.
>>>>>
>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
>>>>> old_aux.  The callers of this function in backref.c, however, pass a
>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
>>>>> variable, in this case, prefs_delayed.
>>>>>
>>>>> Here is a quick attempt to band-aid over this: a new function,
>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
>>>>> value instead of u64.  There are still ugly void ** cast remaining
>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
>>>>> safer than explicit cast to u64, anyway.
>>>>>
>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
>>>>> Cc: <[e-mail]> [v3.11+]
>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
>>>>> ---
>>>>>
>>>>> Alternatively, we can change the argument of aux and old_aux to a
>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
>>>>> function.  I'll cook up another patch if it's the preferred way.
>>>>>
>>>>
>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
>>>
>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
>>> ulist_add_merge() internally.  So, we can't change the type blindly
>>> there, unfortunately.
>>
>> Looking back at the code, it seems that all aux arguments passed to
>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
>> are pointers, so far, and it'd be even cleaner to replace all these
>> from u64 to void *.
>>
>> But, such a replacement patch will become difficult for backporting to
>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
>> should put a smaller fix like my previous one, let it backported to
>> stable kernels, and do more comprehensive replacements to pointer on
>> its top.
>
> Ping.  Could you guys take my original patch as is, or do you prefer
> changing in a different way?  If so, how?
>

I don't care how hard it is to backport to stable, since we're using pointers
everywhere just change it to void * and be done with it.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 3:01 p.m. UTC | #8

At Wed, 30 Jul 2014 10:29:46 -0400,
Josef Bacik wrote:
> 
> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> > At Mon, 28 Jul 2014 16:01:55 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Mon, 28 Jul 2014 15:48:41 +0200,
> >> Takashi Iwai wrote:
> >>>
> >>> At Mon, 28 Jul 2014 09:16:48 -0400,
> >>> Josef Bacik wrote:
> >>>>
> >>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> >>>>> We've got bug reports that btrfs crashes when quota is enabled on
> >>>>> 32bit kernel, typically with the Oops like below:
> >>>>>    BUG: unable to handle kernel NULL pointer dereference at 00000004
> >>>>>    IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>    *pde = 00000000
> >>>>>    Oops: 0000 [#1] SMP
> >>>>>    CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >>>>>    Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >>>>>    task: f1478130 ti: f147c000 task.ti: f147c000
> >>>>>    EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >>>>>    EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>    EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >>>>>    ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >>>>>     DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >>>>>    CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >>>>>    Stack:
> >>>>>     00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >>>>>     00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >>>>>     00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >>>>>    Call Trace:
> >>>>>     [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >>>>>     [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >>>>>     [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >>>>>     [<c025e38b>] process_one_work+0x11b/0x390
> >>>>>     [<c025eea1>] worker_thread+0x101/0x340
> >>>>>     [<c026432b>] kthread+0x9b/0xb0
> >>>>>     [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >>>>>     [<c0264290>] kthread_create_on_node+0x110/0x110
> >>>>>
> >>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> >>>>> investigation and bisection pointed that the call of ulist_add_merge()
> >>>>> results in the corruption.
> >>>>>
> >>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> >>>>> old_aux.  The callers of this function in backref.c, however, pass a
> >>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> >>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> >>>>> variable, in this case, prefs_delayed.
> >>>>>
> >>>>> Here is a quick attempt to band-aid over this: a new function,
> >>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> >>>>> value instead of u64.  There are still ugly void ** cast remaining
> >>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> >>>>> safer than explicit cast to u64, anyway.
> >>>>>
> >>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> >>>>> Cc: <[e-mail]> [v3.11+]
> >>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> >>>>> ---
> >>>>>
> >>>>> Alternatively, we can change the argument of aux and old_aux to a
> >>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> >>>>> function.  I'll cook up another patch if it's the preferred way.
> >>>>>
> >>>>
> >>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> >>>
> >>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> >>> ulist_add_merge() internally.  So, we can't change the type blindly
> >>> there, unfortunately.
> >>
> >> Looking back at the code, it seems that all aux arguments passed to
> >> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> >> are pointers, so far, and it'd be even cleaner to replace all these
> >> from u64 to void *.
> >>
> >> But, such a replacement patch will become difficult for backporting to
> >> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> >> should put a smaller fix like my previous one, let it backported to
> >> stable kernels, and do more comprehensive replacements to pointer on
> >> its top.
> >
> > Ping.  Could you guys take my original patch as is, or do you prefer
> > changing in a different way?  If so, how?
> >
> 
> I don't care how hard it is to backport to stable,

You must do care as a maintainer.  It's a long-standing and serious
bug since 3.11.  The kernel hangs up immediately when you enable quota 
on 32bit kernel.  And it's really hard to revert it when the rootfs is
btrfs.  (The mount follows the immediate hang up after reboot.)

> since we're using pointers
> everywhere just change it to void * and be done with it.  Thanks,

Fix it quickly, then do cleanup.  This is the golden rule for
regression :)


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 3:05 p.m. UTC | #9

At Wed, 30 Jul 2014 17:01:52 +0200,
Takashi Iwai wrote:
> 
> At Wed, 30 Jul 2014 10:29:46 -0400,
> Josef Bacik wrote:
> > 
> > On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> > > At Mon, 28 Jul 2014 16:01:55 +0200,
> > > Takashi Iwai wrote:
> > >>
> > >> At Mon, 28 Jul 2014 15:48:41 +0200,
> > >> Takashi Iwai wrote:
> > >>>
> > >>> At Mon, 28 Jul 2014 09:16:48 -0400,
> > >>> Josef Bacik wrote:
> > >>>>
> > >>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> > >>>>> We've got bug reports that btrfs crashes when quota is enabled on
> > >>>>> 32bit kernel, typically with the Oops like below:
> > >>>>>    BUG: unable to handle kernel NULL pointer dereference at 00000004
> > >>>>>    IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> > >>>>>    *pde = 00000000
> > >>>>>    Oops: 0000 [#1] SMP
> > >>>>>    CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> > >>>>>    Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> > >>>>>    task: f1478130 ti: f147c000 task.ti: f147c000
> > >>>>>    EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> > >>>>>    EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> > >>>>>    EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> > >>>>>    ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> > >>>>>     DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> > >>>>>    CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> > >>>>>    Stack:
> > >>>>>     00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> > >>>>>     00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> > >>>>>     00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> > >>>>>    Call Trace:
> > >>>>>     [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> > >>>>>     [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> > >>>>>     [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> > >>>>>     [<c025e38b>] process_one_work+0x11b/0x390
> > >>>>>     [<c025eea1>] worker_thread+0x101/0x340
> > >>>>>     [<c026432b>] kthread+0x9b/0xb0
> > >>>>>     [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> > >>>>>     [<c0264290>] kthread_create_on_node+0x110/0x110
> > >>>>>
> > >>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> > >>>>> investigation and bisection pointed that the call of ulist_add_merge()
> > >>>>> results in the corruption.
> > >>>>>
> > >>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > >>>>> old_aux.  The callers of this function in backref.c, however, pass a
> > >>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> > >>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > >>>>> variable, in this case, prefs_delayed.
> > >>>>>
> > >>>>> Here is a quick attempt to band-aid over this: a new function,
> > >>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > >>>>> value instead of u64.  There are still ugly void ** cast remaining
> > >>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> > >>>>> safer than explicit cast to u64, anyway.
> > >>>>>
> > >>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> > >>>>> Cc: <[e-mail]> [v3.11+]
> > >>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> > >>>>> ---
> > >>>>>
> > >>>>> Alternatively, we can change the argument of aux and old_aux to a
> > >>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> > >>>>> function.  I'll cook up another patch if it's the preferred way.
> > >>>>>
> > >>>>
> > >>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> > >>>
> > >>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> > >>> ulist_add_merge() internally.  So, we can't change the type blindly
> > >>> there, unfortunately.
> > >>
> > >> Looking back at the code, it seems that all aux arguments passed to
> > >> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> > >> are pointers, so far, and it'd be even cleaner to replace all these
> > >> from u64 to void *.
> > >>
> > >> But, such a replacement patch will become difficult for backporting to
> > >> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> > >> should put a smaller fix like my previous one, let it backported to
> > >> stable kernels, and do more comprehensive replacements to pointer on
> > >> its top.
> > >
> > > Ping.  Could you guys take my original patch as is, or do you prefer
> > > changing in a different way?  If so, how?
> > >
> > 
> > I don't care how hard it is to backport to stable,
> 
> You must do care as a maintainer.  It's a long-standing and serious
> bug since 3.11.  The kernel hangs up immediately when you enable quota 
> on 32bit kernel.  And it's really hard to revert it when the rootfs is
> btrfs.  (The mount follows the immediate hang up after reboot.)
> 
> > since we're using pointers
> > everywhere just change it to void * and be done with it.  Thanks,
> 
> Fix it quickly, then do cleanup.  This is the golden rule for
> regression :)

Also, another question is whether you guys are OK to change the type
to a pointer.  Through a glance, the ulist code was intended to handle
any generic data, thus it uses u64, right?  Using void pointer breaks
this concept.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josef Bacik July 30, 2014, 3:40 p.m. UTC | #10

On 07/30/2014 11:05 AM, Takashi Iwai wrote:
> At Wed, 30 Jul 2014 17:01:52 +0200,
> Takashi Iwai wrote:
>>
>> At Wed, 30 Jul 2014 10:29:46 -0400,
>> Josef Bacik wrote:
>>>
>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
>>>> Takashi Iwai wrote:
>>>>>
>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
>>>>> Takashi Iwai wrote:
>>>>>>
>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
>>>>>> Josef Bacik wrote:
>>>>>>>
>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
>>>>>>>> 32bit kernel, typically with the Oops like below:
>>>>>>>>     BUG: unable to handle kernel NULL pointer dereference at 00000004
>>>>>>>>     IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>     *pde = 00000000
>>>>>>>>     Oops: 0000 [#1] SMP
>>>>>>>>     CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>>>>>>>>     Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>>>>>>>>     task: f1478130 ti: f147c000 task.ti: f147c000
>>>>>>>>     EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>>>>>>>>     EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>     EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>>>>>>>>     ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>>>>>>>>      DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>>>>>>>>     CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>>>>>>>>     Stack:
>>>>>>>>      00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>>>>>>>>      00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>>>>>>>>      00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>>>>>>>>     Call Trace:
>>>>>>>>      [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>>>>>>>>      [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>>>>>>>>      [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>>>>>>>>      [<c025e38b>] process_one_work+0x11b/0x390
>>>>>>>>      [<c025eea1>] worker_thread+0x101/0x340
>>>>>>>>      [<c026432b>] kthread+0x9b/0xb0
>>>>>>>>      [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>>>>>>>>      [<c0264290>] kthread_create_on_node+0x110/0x110
>>>>>>>>
>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
>>>>>>>> results in the corruption.
>>>>>>>>
>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
>>>>>>>> variable, in this case, prefs_delayed.
>>>>>>>>
>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
>>>>>>>> safer than explicit cast to u64, anyway.
>>>>>>>>
>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
>>>>>>>> Cc: <[e-mail]> [v3.11+]
>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
>>>>>>>>
>>>>>>>
>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
>>>>>>
>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
>>>>>> there, unfortunately.
>>>>>
>>>>> Looking back at the code, it seems that all aux arguments passed to
>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
>>>>> are pointers, so far, and it'd be even cleaner to replace all these
>>>>> from u64 to void *.
>>>>>
>>>>> But, such a replacement patch will become difficult for backporting to
>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
>>>>> should put a smaller fix like my previous one, let it backported to
>>>>> stable kernels, and do more comprehensive replacements to pointer on
>>>>> its top.
>>>>
>>>> Ping.  Could you guys take my original patch as is, or do you prefer
>>>> changing in a different way?  If so, how?
>>>>
>>>
>>> I don't care how hard it is to backport to stable,
>>
>> You must do care as a maintainer.  It's a long-standing and serious
>> bug since 3.11.  The kernel hangs up immediately when you enable quota
>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
>> btrfs.  (The mount follows the immediate hang up after reboot.)
>>

What I mean is that we want the right fix first, not something that is easier to
pull back to stable and then the right fix later.  Do it right first and then
backport it to the stable kernels, it's perfectly acceptable to adjust patches
when sending them to the stable team.  "But it's hard" is not a valid excuse for
not doing it right the first time.

>>> since we're using pointers
>>> everywhere just change it to void * and be done with it.  Thanks,
>>
>> Fix it quickly, then do cleanup.  This is the golden rule for
>> regression :)
>
> Also, another question is whether you guys are OK to change the type
> to a pointer.  Through a glance, the ulist code was intended to handle
> any generic data, thus it uses u64, right?  Using void pointer breaks
> this concept.
>

It's fine, ulist today resembles very little from what it was originally.  The
current users all shove pointers into there, so we might as well just make it a
pointer.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 3:52 p.m. UTC | #11

At Wed, 30 Jul 2014 11:40:14 -0400,
Josef Bacik wrote:
> 
> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
> > At Wed, 30 Jul 2014 17:01:52 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Wed, 30 Jul 2014 10:29:46 -0400,
> >> Josef Bacik wrote:
> >>>
> >>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> >>>> At Mon, 28 Jul 2014 16:01:55 +0200,
> >>>> Takashi Iwai wrote:
> >>>>>
> >>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
> >>>>> Takashi Iwai wrote:
> >>>>>>
> >>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
> >>>>>> Josef Bacik wrote:
> >>>>>>>
> >>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> >>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
> >>>>>>>> 32bit kernel, typically with the Oops like below:
> >>>>>>>>     BUG: unable to handle kernel NULL pointer dereference at 00000004
> >>>>>>>>     IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>     *pde = 00000000
> >>>>>>>>     Oops: 0000 [#1] SMP
> >>>>>>>>     CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >>>>>>>>     Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >>>>>>>>     task: f1478130 ti: f147c000 task.ti: f147c000
> >>>>>>>>     EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >>>>>>>>     EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>     EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >>>>>>>>     ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >>>>>>>>      DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >>>>>>>>     CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >>>>>>>>     Stack:
> >>>>>>>>      00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >>>>>>>>      00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >>>>>>>>      00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >>>>>>>>     Call Trace:
> >>>>>>>>      [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >>>>>>>>      [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >>>>>>>>      [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >>>>>>>>      [<c025e38b>] process_one_work+0x11b/0x390
> >>>>>>>>      [<c025eea1>] worker_thread+0x101/0x340
> >>>>>>>>      [<c026432b>] kthread+0x9b/0xb0
> >>>>>>>>      [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >>>>>>>>      [<c0264290>] kthread_create_on_node+0x110/0x110
> >>>>>>>>
> >>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> >>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
> >>>>>>>> results in the corruption.
> >>>>>>>>
> >>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> >>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
> >>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> >>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> >>>>>>>> variable, in this case, prefs_delayed.
> >>>>>>>>
> >>>>>>>> Here is a quick attempt to band-aid over this: a new function,
> >>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> >>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
> >>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> >>>>>>>> safer than explicit cast to u64, anyway.
> >>>>>>>>
> >>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> >>>>>>>> Cc: <[e-mail]> [v3.11+]
> >>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
> >>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> >>>>>>>> function.  I'll cook up another patch if it's the preferred way.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> >>>>>>
> >>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> >>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
> >>>>>> there, unfortunately.
> >>>>>
> >>>>> Looking back at the code, it seems that all aux arguments passed to
> >>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> >>>>> are pointers, so far, and it'd be even cleaner to replace all these
> >>>>> from u64 to void *.
> >>>>>
> >>>>> But, such a replacement patch will become difficult for backporting to
> >>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> >>>>> should put a smaller fix like my previous one, let it backported to
> >>>>> stable kernels, and do more comprehensive replacements to pointer on
> >>>>> its top.
> >>>>
> >>>> Ping.  Could you guys take my original patch as is, or do you prefer
> >>>> changing in a different way?  If so, how?
> >>>>
> >>>
> >>> I don't care how hard it is to backport to stable,
> >>
> >> You must do care as a maintainer.  It's a long-standing and serious
> >> bug since 3.11.  The kernel hangs up immediately when you enable quota
> >> on 32bit kernel.  And it's really hard to revert it when the rootfs is
> >> btrfs.  (The mount follows the immediate hang up after reboot.)
> >>
> 
> What I mean is that we want the right fix first, not something that is easier to
> pull back to stable and then the right fix later.  Do it right first and then
> backport it to the stable kernels, it's perfectly acceptable to adjust patches
> when sending them to the stable team.  "But it's hard" is not a valid excuse for
> not doing it right the first time.

Well, I guess this underestimates the burden of backports.  Currently,
there is stable kernel for each kernel release.  Anyone has to
backport for each version, and you'll be asked.  I, as a long-time
subsystem maintainer, wouldn't go in that way :)

And, speaking of "rightness" -- replacing the callers with a wrapper
is also a right fix.  It's even a safer fix.  That's basically why I
posted it as the primary patch.

The merit of replacing all callers is that you can eliminate nasty
casts by that.  This is however rather a cleanup, which is a different
bonus from what we need to fix.

> >>> since we're using pointers
> >>> everywhere just change it to void * and be done with it.  Thanks,
> >>
> >> Fix it quickly, then do cleanup.  This is the golden rule for
> >> regression :)
> >
> > Also, another question is whether you guys are OK to change the type
> > to a pointer.  Through a glance, the ulist code was intended to handle
> > any generic data, thus it uses u64, right?  Using void pointer breaks
> > this concept.
> >
> 
> It's fine, ulist today resembles very little from what it was originally.  The
> current users all shove pointers into there, so we might as well just make it a
> pointer.  Thanks,

OK, good to know.

If this post still doesn't convince you, I'll prepare the patch to do
all replacements.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josef Bacik July 30, 2014, 4:01 p.m. UTC | #12

On 07/30/2014 11:52 AM, Takashi Iwai wrote:
> At Wed, 30 Jul 2014 11:40:14 -0400,
> Josef Bacik wrote:
>>
>> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
>>> At Wed, 30 Jul 2014 17:01:52 +0200,
>>> Takashi Iwai wrote:
>>>>
>>>> At Wed, 30 Jul 2014 10:29:46 -0400,
>>>> Josef Bacik wrote:
>>>>>
>>>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
>>>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
>>>>>> Takashi Iwai wrote:
>>>>>>>
>>>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
>>>>>>> Takashi Iwai wrote:
>>>>>>>>
>>>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
>>>>>>>> Josef Bacik wrote:
>>>>>>>>>
>>>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
>>>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
>>>>>>>>>> 32bit kernel, typically with the Oops like below:
>>>>>>>>>>      BUG: unable to handle kernel NULL pointer dereference at 00000004
>>>>>>>>>>      IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>>>      *pde = 00000000
>>>>>>>>>>      Oops: 0000 [#1] SMP
>>>>>>>>>>      CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>>>>>>>>>>      Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>>>>>>>>>>      task: f1478130 ti: f147c000 task.ti: f147c000
>>>>>>>>>>      EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>>>>>>>>>>      EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>>>      EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>>>>>>>>>>      ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>>>>>>>>>>       DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>>>>>>>>>>      CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>>>>>>>>>>      Stack:
>>>>>>>>>>       00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>>>>>>>>>>       00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>>>>>>>>>>       00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>>>>>>>>>>      Call Trace:
>>>>>>>>>>       [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>>>>>>>>>>       [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>>>>>>>>>>       [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>>>>>>>>>>       [<c025e38b>] process_one_work+0x11b/0x390
>>>>>>>>>>       [<c025eea1>] worker_thread+0x101/0x340
>>>>>>>>>>       [<c026432b>] kthread+0x9b/0xb0
>>>>>>>>>>       [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>>>>>>>>>>       [<c0264290>] kthread_create_on_node+0x110/0x110
>>>>>>>>>>
>>>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
>>>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
>>>>>>>>>> results in the corruption.
>>>>>>>>>>
>>>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
>>>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
>>>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
>>>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
>>>>>>>>>> variable, in this case, prefs_delayed.
>>>>>>>>>>
>>>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
>>>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
>>>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
>>>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
>>>>>>>>>> safer than explicit cast to u64, anyway.
>>>>>>>>>>
>>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
>>>>>>>>>> Cc: <[e-mail]> [v3.11+]
>>>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
>>>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
>>>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
>>>>>>>>
>>>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
>>>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
>>>>>>>> there, unfortunately.
>>>>>>>
>>>>>>> Looking back at the code, it seems that all aux arguments passed to
>>>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
>>>>>>> are pointers, so far, and it'd be even cleaner to replace all these
>>>>>>> from u64 to void *.
>>>>>>>
>>>>>>> But, such a replacement patch will become difficult for backporting to
>>>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
>>>>>>> should put a smaller fix like my previous one, let it backported to
>>>>>>> stable kernels, and do more comprehensive replacements to pointer on
>>>>>>> its top.
>>>>>>
>>>>>> Ping.  Could you guys take my original patch as is, or do you prefer
>>>>>> changing in a different way?  If so, how?
>>>>>>
>>>>>
>>>>> I don't care how hard it is to backport to stable,
>>>>
>>>> You must do care as a maintainer.  It's a long-standing and serious
>>>> bug since 3.11.  The kernel hangs up immediately when you enable quota
>>>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
>>>> btrfs.  (The mount follows the immediate hang up after reboot.)
>>>>
>>
>> What I mean is that we want the right fix first, not something that is easier to
>> pull back to stable and then the right fix later.  Do it right first and then
>> backport it to the stable kernels, it's perfectly acceptable to adjust patches
>> when sending them to the stable team.  "But it's hard" is not a valid excuse for
>> not doing it right the first time.
>
> Well, I guess this underestimates the burden of backports.  Currently,
> there is stable kernel for each kernel release.  Anyone has to
> backport for each version, and you'll be asked.  I, as a long-time
> subsystem maintainer, wouldn't go in that way :)
>
> And, speaking of "rightness" -- replacing the callers with a wrapper
> is also a right fix.  It's even a safer fix.  That's basically why I
> posted it as the primary patch.
>
> The merit of replacing all callers is that you can eliminate nasty
> casts by that.  This is however rather a cleanup, which is a different
> bonus from what we need to fix.
>
>>>>> since we're using pointers
>>>>> everywhere just change it to void * and be done with it.  Thanks,
>>>>
>>>> Fix it quickly, then do cleanup.  This is the golden rule for
>>>> regression :)
>>>
>>> Also, another question is whether you guys are OK to change the type
>>> to a pointer.  Through a glance, the ulist code was intended to handle
>>> any generic data, thus it uses u64, right?  Using void pointer breaks
>>> this concept.
>>>
>>
>> It's fine, ulist today resembles very little from what it was originally.  The
>> current users all shove pointers into there, so we might as well just make it a
>> pointer.  Thanks,
>
> OK, good to know.
>
> If this post still doesn't convince you, I'll prepare the patch to do
> all replacements.
>
>

I don't care that much, do it however you want.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 4:35 p.m. UTC | #13

At Wed, 30 Jul 2014 12:01:31 -0400,
Josef Bacik wrote:
> 
> On 07/30/2014 11:52 AM, Takashi Iwai wrote:
> > At Wed, 30 Jul 2014 11:40:14 -0400,
> > Josef Bacik wrote:
> >>
> >> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
> >>> At Wed, 30 Jul 2014 17:01:52 +0200,
> >>> Takashi Iwai wrote:
> >>>>
> >>>> At Wed, 30 Jul 2014 10:29:46 -0400,
> >>>> Josef Bacik wrote:
> >>>>>
> >>>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> >>>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
> >>>>>> Takashi Iwai wrote:
> >>>>>>>
> >>>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
> >>>>>>> Takashi Iwai wrote:
> >>>>>>>>
> >>>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
> >>>>>>>> Josef Bacik wrote:
> >>>>>>>>>
> >>>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> >>>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
> >>>>>>>>>> 32bit kernel, typically with the Oops like below:
> >>>>>>>>>>      BUG: unable to handle kernel NULL pointer dereference at 00000004
> >>>>>>>>>>      IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>>>      *pde = 00000000
> >>>>>>>>>>      Oops: 0000 [#1] SMP
> >>>>>>>>>>      CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >>>>>>>>>>      Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >>>>>>>>>>      task: f1478130 ti: f147c000 task.ti: f147c000
> >>>>>>>>>>      EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >>>>>>>>>>      EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>>>      EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >>>>>>>>>>      ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >>>>>>>>>>       DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >>>>>>>>>>      CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >>>>>>>>>>      Stack:
> >>>>>>>>>>       00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >>>>>>>>>>       00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >>>>>>>>>>       00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >>>>>>>>>>      Call Trace:
> >>>>>>>>>>       [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >>>>>>>>>>       [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >>>>>>>>>>       [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >>>>>>>>>>       [<c025e38b>] process_one_work+0x11b/0x390
> >>>>>>>>>>       [<c025eea1>] worker_thread+0x101/0x340
> >>>>>>>>>>       [<c026432b>] kthread+0x9b/0xb0
> >>>>>>>>>>       [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >>>>>>>>>>       [<c0264290>] kthread_create_on_node+0x110/0x110
> >>>>>>>>>>
> >>>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> >>>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
> >>>>>>>>>> results in the corruption.
> >>>>>>>>>>
> >>>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> >>>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
> >>>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> >>>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> >>>>>>>>>> variable, in this case, prefs_delayed.
> >>>>>>>>>>
> >>>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
> >>>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> >>>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
> >>>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> >>>>>>>>>> safer than explicit cast to u64, anyway.
> >>>>>>>>>>
> >>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> >>>>>>>>>> Cc: <[e-mail]> [v3.11+]
> >>>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
> >>>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> >>>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> >>>>>>>>
> >>>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> >>>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
> >>>>>>>> there, unfortunately.
> >>>>>>>
> >>>>>>> Looking back at the code, it seems that all aux arguments passed to
> >>>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> >>>>>>> are pointers, so far, and it'd be even cleaner to replace all these
> >>>>>>> from u64 to void *.
> >>>>>>>
> >>>>>>> But, such a replacement patch will become difficult for backporting to
> >>>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> >>>>>>> should put a smaller fix like my previous one, let it backported to
> >>>>>>> stable kernels, and do more comprehensive replacements to pointer on
> >>>>>>> its top.
> >>>>>>
> >>>>>> Ping.  Could you guys take my original patch as is, or do you prefer
> >>>>>> changing in a different way?  If so, how?
> >>>>>>
> >>>>>
> >>>>> I don't care how hard it is to backport to stable,
> >>>>
> >>>> You must do care as a maintainer.  It's a long-standing and serious
> >>>> bug since 3.11.  The kernel hangs up immediately when you enable quota
> >>>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
> >>>> btrfs.  (The mount follows the immediate hang up after reboot.)
> >>>>
> >>
> >> What I mean is that we want the right fix first, not something that is easier to
> >> pull back to stable and then the right fix later.  Do it right first and then
> >> backport it to the stable kernels, it's perfectly acceptable to adjust patches
> >> when sending them to the stable team.  "But it's hard" is not a valid excuse for
> >> not doing it right the first time.
> >
> > Well, I guess this underestimates the burden of backports.  Currently,
> > there is stable kernel for each kernel release.  Anyone has to
> > backport for each version, and you'll be asked.  I, as a long-time
> > subsystem maintainer, wouldn't go in that way :)
> >
> > And, speaking of "rightness" -- replacing the callers with a wrapper
> > is also a right fix.  It's even a safer fix.  That's basically why I
> > posted it as the primary patch.
> >
> > The merit of replacing all callers is that you can eliminate nasty
> > casts by that.  This is however rather a cleanup, which is a different
> > bonus from what we need to fix.
> >
> >>>>> since we're using pointers
> >>>>> everywhere just change it to void * and be done with it.  Thanks,
> >>>>
> >>>> Fix it quickly, then do cleanup.  This is the golden rule for
> >>>> regression :)
> >>>
> >>> Also, another question is whether you guys are OK to change the type
> >>> to a pointer.  Through a glance, the ulist code was intended to handle
> >>> any generic data, thus it uses u64, right?  Using void pointer breaks
> >>> this concept.
> >>>
> >>
> >> It's fine, ulist today resembles very little from what it was originally.  The
> >> current users all shove pointers into there, so we might as well just make it a
> >> pointer.  Thanks,
> >
> > OK, good to know.
> >
> > If this post still doesn't convince you, I'll prepare the patch to do
> > all replacements.
> >
> >
> 
> I don't care that much, do it however you want.  Thanks,

Yes, I do care because I know of this kind of horror very well.

OK, I'm going to send the new patch(es).


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Josef Bacik July 30, 2014, 5 p.m. UTC | #14

On 07/30/2014 12:35 PM, Takashi Iwai wrote:
> At Wed, 30 Jul 2014 12:01:31 -0400,
> Josef Bacik wrote:
>>
>> On 07/30/2014 11:52 AM, Takashi Iwai wrote:
>>> At Wed, 30 Jul 2014 11:40:14 -0400,
>>> Josef Bacik wrote:
>>>>
>>>> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
>>>>> At Wed, 30 Jul 2014 17:01:52 +0200,
>>>>> Takashi Iwai wrote:
>>>>>>
>>>>>> At Wed, 30 Jul 2014 10:29:46 -0400,
>>>>>> Josef Bacik wrote:
>>>>>>>
>>>>>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
>>>>>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
>>>>>>>> Takashi Iwai wrote:
>>>>>>>>>
>>>>>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
>>>>>>>>> Takashi Iwai wrote:
>>>>>>>>>>
>>>>>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
>>>>>>>>>> Josef Bacik wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
>>>>>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
>>>>>>>>>>>> 32bit kernel, typically with the Oops like below:
>>>>>>>>>>>>       BUG: unable to handle kernel NULL pointer dereference at 00000004
>>>>>>>>>>>>       IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>>>>>       *pde = 00000000
>>>>>>>>>>>>       Oops: 0000 [#1] SMP
>>>>>>>>>>>>       CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
>>>>>>>>>>>>       Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
>>>>>>>>>>>>       task: f1478130 ti: f147c000 task.ti: f147c000
>>>>>>>>>>>>       EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
>>>>>>>>>>>>       EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
>>>>>>>>>>>>       EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
>>>>>>>>>>>>       ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
>>>>>>>>>>>>        DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>>>>>>>>>>>>       CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
>>>>>>>>>>>>       Stack:
>>>>>>>>>>>>        00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
>>>>>>>>>>>>        00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
>>>>>>>>>>>>        00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
>>>>>>>>>>>>       Call Trace:
>>>>>>>>>>>>        [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
>>>>>>>>>>>>        [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
>>>>>>>>>>>>        [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
>>>>>>>>>>>>        [<c025e38b>] process_one_work+0x11b/0x390
>>>>>>>>>>>>        [<c025eea1>] worker_thread+0x101/0x340
>>>>>>>>>>>>        [<c026432b>] kthread+0x9b/0xb0
>>>>>>>>>>>>        [<c0712a71>] ret_from_kernel_thread+0x21/0x30
>>>>>>>>>>>>        [<c0264290>] kthread_create_on_node+0x110/0x110
>>>>>>>>>>>>
>>>>>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
>>>>>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
>>>>>>>>>>>> results in the corruption.
>>>>>>>>>>>>
>>>>>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
>>>>>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
>>>>>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
>>>>>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
>>>>>>>>>>>> variable, in this case, prefs_delayed.
>>>>>>>>>>>>
>>>>>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
>>>>>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
>>>>>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
>>>>>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
>>>>>>>>>>>> safer than explicit cast to u64, anyway.
>>>>>>>>>>>>
>>>>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
>>>>>>>>>>>> Cc: <[e-mail]> [v3.11+]
>>>>>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
>>>>>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
>>>>>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
>>>>>>>>>>
>>>>>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
>>>>>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
>>>>>>>>>> there, unfortunately.
>>>>>>>>>
>>>>>>>>> Looking back at the code, it seems that all aux arguments passed to
>>>>>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
>>>>>>>>> are pointers, so far, and it'd be even cleaner to replace all these
>>>>>>>>> from u64 to void *.
>>>>>>>>>
>>>>>>>>> But, such a replacement patch will become difficult for backporting to
>>>>>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
>>>>>>>>> should put a smaller fix like my previous one, let it backported to
>>>>>>>>> stable kernels, and do more comprehensive replacements to pointer on
>>>>>>>>> its top.
>>>>>>>>
>>>>>>>> Ping.  Could you guys take my original patch as is, or do you prefer
>>>>>>>> changing in a different way?  If so, how?
>>>>>>>>
>>>>>>>
>>>>>>> I don't care how hard it is to backport to stable,
>>>>>>
>>>>>> You must do care as a maintainer.  It's a long-standing and serious
>>>>>> bug since 3.11.  The kernel hangs up immediately when you enable quota
>>>>>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
>>>>>> btrfs.  (The mount follows the immediate hang up after reboot.)
>>>>>>
>>>>
>>>> What I mean is that we want the right fix first, not something that is easier to
>>>> pull back to stable and then the right fix later.  Do it right first and then
>>>> backport it to the stable kernels, it's perfectly acceptable to adjust patches
>>>> when sending them to the stable team.  "But it's hard" is not a valid excuse for
>>>> not doing it right the first time.
>>>
>>> Well, I guess this underestimates the burden of backports.  Currently,
>>> there is stable kernel for each kernel release.  Anyone has to
>>> backport for each version, and you'll be asked.  I, as a long-time
>>> subsystem maintainer, wouldn't go in that way :)
>>>
>>> And, speaking of "rightness" -- replacing the callers with a wrapper
>>> is also a right fix.  It's even a safer fix.  That's basically why I
>>> posted it as the primary patch.
>>>
>>> The merit of replacing all callers is that you can eliminate nasty
>>> casts by that.  This is however rather a cleanup, which is a different
>>> bonus from what we need to fix.
>>>
>>>>>>> since we're using pointers
>>>>>>> everywhere just change it to void * and be done with it.  Thanks,
>>>>>>
>>>>>> Fix it quickly, then do cleanup.  This is the golden rule for
>>>>>> regression :)
>>>>>
>>>>> Also, another question is whether you guys are OK to change the type
>>>>> to a pointer.  Through a glance, the ulist code was intended to handle
>>>>> any generic data, thus it uses u64, right?  Using void pointer breaks
>>>>> this concept.
>>>>>
>>>>
>>>> It's fine, ulist today resembles very little from what it was originally.  The
>>>> current users all shove pointers into there, so we might as well just make it a
>>>> pointer.  Thanks,
>>>
>>> OK, good to know.
>>>
>>> If this post still doesn't convince you, I'll prepare the patch to do
>>> all replacements.
>>>
>>>
>>
>> I don't care that much, do it however you want.  Thanks,
>
> Yes, I do care because I know of this kind of horror very well.
>
> OK, I'm going to send the new patch(es).
>

I meant that I don't care how you do it, if you want to do it the simple way
first and then send the cleanup later thats fine by me.  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai July 30, 2014, 5:45 p.m. UTC | #15

At Wed, 30 Jul 2014 13:00:24 -0400,
Josef Bacik wrote:
> 
> On 07/30/2014 12:35 PM, Takashi Iwai wrote:
> > At Wed, 30 Jul 2014 12:01:31 -0400,
> > Josef Bacik wrote:
> >>
> >> On 07/30/2014 11:52 AM, Takashi Iwai wrote:
> >>> At Wed, 30 Jul 2014 11:40:14 -0400,
> >>> Josef Bacik wrote:
> >>>>
> >>>> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
> >>>>> At Wed, 30 Jul 2014 17:01:52 +0200,
> >>>>> Takashi Iwai wrote:
> >>>>>>
> >>>>>> At Wed, 30 Jul 2014 10:29:46 -0400,
> >>>>>> Josef Bacik wrote:
> >>>>>>>
> >>>>>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> >>>>>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
> >>>>>>>> Takashi Iwai wrote:
> >>>>>>>>>
> >>>>>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
> >>>>>>>>> Takashi Iwai wrote:
> >>>>>>>>>>
> >>>>>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
> >>>>>>>>>> Josef Bacik wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> >>>>>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
> >>>>>>>>>>>> 32bit kernel, typically with the Oops like below:
> >>>>>>>>>>>>       BUG: unable to handle kernel NULL pointer dereference at 00000004
> >>>>>>>>>>>>       IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>>>>>       *pde = 00000000
> >>>>>>>>>>>>       Oops: 0000 [#1] SMP
> >>>>>>>>>>>>       CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> >>>>>>>>>>>>       Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> >>>>>>>>>>>>       task: f1478130 ti: f147c000 task.ti: f147c000
> >>>>>>>>>>>>       EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> >>>>>>>>>>>>       EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> >>>>>>>>>>>>       EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> >>>>>>>>>>>>       ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> >>>>>>>>>>>>        DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> >>>>>>>>>>>>       CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> >>>>>>>>>>>>       Stack:
> >>>>>>>>>>>>        00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> >>>>>>>>>>>>        00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> >>>>>>>>>>>>        00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> >>>>>>>>>>>>       Call Trace:
> >>>>>>>>>>>>        [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> >>>>>>>>>>>>        [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> >>>>>>>>>>>>        [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> >>>>>>>>>>>>        [<c025e38b>] process_one_work+0x11b/0x390
> >>>>>>>>>>>>        [<c025eea1>] worker_thread+0x101/0x340
> >>>>>>>>>>>>        [<c026432b>] kthread+0x9b/0xb0
> >>>>>>>>>>>>        [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> >>>>>>>>>>>>        [<c0264290>] kthread_create_on_node+0x110/0x110
> >>>>>>>>>>>>
> >>>>>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> >>>>>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
> >>>>>>>>>>>> results in the corruption.
> >>>>>>>>>>>>
> >>>>>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> >>>>>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
> >>>>>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> >>>>>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> >>>>>>>>>>>> variable, in this case, prefs_delayed.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
> >>>>>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> >>>>>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
> >>>>>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> >>>>>>>>>>>> safer than explicit cast to u64, anyway.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> >>>>>>>>>>>> Cc: <[e-mail]> [v3.11+]
> >>>>>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>
> >>>>>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
> >>>>>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> >>>>>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> >>>>>>>>>>
> >>>>>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> >>>>>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
> >>>>>>>>>> there, unfortunately.
> >>>>>>>>>
> >>>>>>>>> Looking back at the code, it seems that all aux arguments passed to
> >>>>>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> >>>>>>>>> are pointers, so far, and it'd be even cleaner to replace all these
> >>>>>>>>> from u64 to void *.
> >>>>>>>>>
> >>>>>>>>> But, such a replacement patch will become difficult for backporting to
> >>>>>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> >>>>>>>>> should put a smaller fix like my previous one, let it backported to
> >>>>>>>>> stable kernels, and do more comprehensive replacements to pointer on
> >>>>>>>>> its top.
> >>>>>>>>
> >>>>>>>> Ping.  Could you guys take my original patch as is, or do you prefer
> >>>>>>>> changing in a different way?  If so, how?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I don't care how hard it is to backport to stable,
> >>>>>>
> >>>>>> You must do care as a maintainer.  It's a long-standing and serious
> >>>>>> bug since 3.11.  The kernel hangs up immediately when you enable quota
> >>>>>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
> >>>>>> btrfs.  (The mount follows the immediate hang up after reboot.)
> >>>>>>
> >>>>
> >>>> What I mean is that we want the right fix first, not something that is easier to
> >>>> pull back to stable and then the right fix later.  Do it right first and then
> >>>> backport it to the stable kernels, it's perfectly acceptable to adjust patches
> >>>> when sending them to the stable team.  "But it's hard" is not a valid excuse for
> >>>> not doing it right the first time.
> >>>
> >>> Well, I guess this underestimates the burden of backports.  Currently,
> >>> there is stable kernel for each kernel release.  Anyone has to
> >>> backport for each version, and you'll be asked.  I, as a long-time
> >>> subsystem maintainer, wouldn't go in that way :)
> >>>
> >>> And, speaking of "rightness" -- replacing the callers with a wrapper
> >>> is also a right fix.  It's even a safer fix.  That's basically why I
> >>> posted it as the primary patch.
> >>>
> >>> The merit of replacing all callers is that you can eliminate nasty
> >>> casts by that.  This is however rather a cleanup, which is a different
> >>> bonus from what we need to fix.
> >>>
> >>>>>>> since we're using pointers
> >>>>>>> everywhere just change it to void * and be done with it.  Thanks,
> >>>>>>
> >>>>>> Fix it quickly, then do cleanup.  This is the golden rule for
> >>>>>> regression :)
> >>>>>
> >>>>> Also, another question is whether you guys are OK to change the type
> >>>>> to a pointer.  Through a glance, the ulist code was intended to handle
> >>>>> any generic data, thus it uses u64, right?  Using void pointer breaks
> >>>>> this concept.
> >>>>>
> >>>>
> >>>> It's fine, ulist today resembles very little from what it was originally.  The
> >>>> current users all shove pointers into there, so we might as well just make it a
> >>>> pointer.  Thanks,
> >>>
> >>> OK, good to know.
> >>>
> >>> If this post still doesn't convince you, I'll prepare the patch to do
> >>> all replacements.
> >>>
> >>>
> >>
> >> I don't care that much, do it however you want.  Thanks,
> >
> > Yes, I do care because I know of this kind of horror very well.
> >
> > OK, I'm going to send the new patch(es).
> >
> 
> I meant that I don't care how you do it, if you want to do it the simple way
> first and then send the cleanup later thats fine by me.  Thanks,

Ah, misunderstood.  Then please disregard my v2 patches and take just
v1 patch.  It's even good for 3.16, if possible.

I'll send the cleanup patch on top of that.


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai Aug. 6, 2014, 3:14 p.m. UTC | #16

At Wed, 30 Jul 2014 19:45:36 +0200,
Takashi Iwai wrote:
> 
> At Wed, 30 Jul 2014 13:00:24 -0400,
> Josef Bacik wrote:
> > 
> > On 07/30/2014 12:35 PM, Takashi Iwai wrote:
> > > At Wed, 30 Jul 2014 12:01:31 -0400,
> > > Josef Bacik wrote:
> > >>
> > >> On 07/30/2014 11:52 AM, Takashi Iwai wrote:
> > >>> At Wed, 30 Jul 2014 11:40:14 -0400,
> > >>> Josef Bacik wrote:
> > >>>>
> > >>>> On 07/30/2014 11:05 AM, Takashi Iwai wrote:
> > >>>>> At Wed, 30 Jul 2014 17:01:52 +0200,
> > >>>>> Takashi Iwai wrote:
> > >>>>>>
> > >>>>>> At Wed, 30 Jul 2014 10:29:46 -0400,
> > >>>>>> Josef Bacik wrote:
> > >>>>>>>
> > >>>>>>> On 07/30/2014 05:57 AM, Takashi Iwai wrote:
> > >>>>>>>> At Mon, 28 Jul 2014 16:01:55 +0200,
> > >>>>>>>> Takashi Iwai wrote:
> > >>>>>>>>>
> > >>>>>>>>> At Mon, 28 Jul 2014 15:48:41 +0200,
> > >>>>>>>>> Takashi Iwai wrote:
> > >>>>>>>>>>
> > >>>>>>>>>> At Mon, 28 Jul 2014 09:16:48 -0400,
> > >>>>>>>>>> Josef Bacik wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>> On 07/28/2014 04:57 AM, Takashi Iwai wrote:
> > >>>>>>>>>>>> We've got bug reports that btrfs crashes when quota is enabled on
> > >>>>>>>>>>>> 32bit kernel, typically with the Oops like below:
> > >>>>>>>>>>>>       BUG: unable to handle kernel NULL pointer dereference at 00000004
> > >>>>>>>>>>>>       IP: [<f9234590>] find_parent_nodes+0x360/0x1380 [btrfs]
> > >>>>>>>>>>>>       *pde = 00000000
> > >>>>>>>>>>>>       Oops: 0000 [#1] SMP
> > >>>>>>>>>>>>       CPU: 0 PID: 151 Comm: kworker/u8:2 Tainted: G S      W 3.15.2-1.gd43d97e-default #1
> > >>>>>>>>>>>>       Workqueue: btrfs-qgroup-rescan normal_work_helper [btrfs]
> > >>>>>>>>>>>>       task: f1478130 ti: f147c000 task.ti: f147c000
> > >>>>>>>>>>>>       EIP: 0060:[<f9234590>] EFLAGS: 00010213 CPU: 0
> > >>>>>>>>>>>>       EIP is at find_parent_nodes+0x360/0x1380 [btrfs]
> > >>>>>>>>>>>>       EAX: f147dda8 EBX: f147ddb0 ECX: 00000011 EDX: 00000000
> > >>>>>>>>>>>>       ESI: 00000000 EDI: f147dda4 EBP: f147ddf8 ESP: f147dd38
> > >>>>>>>>>>>>        DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> > >>>>>>>>>>>>       CR0: 8005003b CR2: 00000004 CR3: 00bf3000 CR4: 00000690
> > >>>>>>>>>>>>       Stack:
> > >>>>>>>>>>>>        00000000 00000000 f147dda4 00000050 00000001 00000000 00000001 00000050
> > >>>>>>>>>>>>        00000001 00000000 d3059000 00000001 00000022 000000a8 00000000 00000000
> > >>>>>>>>>>>>        00000000 000000a1 00000000 00000000 00000001 00000000 00000000 11800000
> > >>>>>>>>>>>>       Call Trace:
> > >>>>>>>>>>>>        [<f923564d>] __btrfs_find_all_roots+0x9d/0xf0 [btrfs]
> > >>>>>>>>>>>>        [<f9237bb1>] btrfs_qgroup_rescan_worker+0x401/0x760 [btrfs]
> > >>>>>>>>>>>>        [<f9206148>] normal_work_helper+0xc8/0x270 [btrfs]
> > >>>>>>>>>>>>        [<c025e38b>] process_one_work+0x11b/0x390
> > >>>>>>>>>>>>        [<c025eea1>] worker_thread+0x101/0x340
> > >>>>>>>>>>>>        [<c026432b>] kthread+0x9b/0xb0
> > >>>>>>>>>>>>        [<c0712a71>] ret_from_kernel_thread+0x21/0x30
> > >>>>>>>>>>>>        [<c0264290>] kthread_create_on_node+0x110/0x110
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> This indicates a NULL corruption in prefs_delayed list.  The further
> > >>>>>>>>>>>> investigation and bisection pointed that the call of ulist_add_merge()
> > >>>>>>>>>>>> results in the corruption.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> ulist_add_merge() takes u64 as aux and writes a 64bit value into
> > >>>>>>>>>>>> old_aux.  The callers of this function in backref.c, however, pass a
> > >>>>>>>>>>>> pointer of a pointer to old_aux.  That is, the function overwrites
> > >>>>>>>>>>>> 64bit value on 32bit pointer.  This caused a NULL in the adjacent
> > >>>>>>>>>>>> variable, in this case, prefs_delayed.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Here is a quick attempt to band-aid over this: a new function,
> > >>>>>>>>>>>> ulist_add_merge_ptr() is introduced to pass/store properly a pointer
> > >>>>>>>>>>>> value instead of u64.  There are still ugly void ** cast remaining
> > >>>>>>>>>>>> in the callers because void ** cannot be taken implicitly.  But, it's
> > >>>>>>>>>>>> safer than explicit cast to u64, anyway.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.novell.com/show_bug.cgi?id%3D887046&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=cKCbChRKsMpTX8ybrSkonQ%3D%3D%0A&m=m3qrbo6ngjqKO%2B7ofuwRfQflb9Cx%2FXrF8TKejkPjxfA%3D%0A&s=199a5b6f0ed181925e9ba2c1060fe20d1c8ad2831dd1d96cc7eddd2a343fa72b
> > >>>>>>>>>>>> Cc: <[e-mail]> [v3.11+]
> > >>>>>>>>>>>> Signed-off-by: Takashi Iwai <[e-mail]>
> > >>>>>>>>>>>> ---
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Alternatively, we can change the argument of aux and old_aux to a
> > >>>>>>>>>>>> pointer from u64, as backref.c is the only user of ulist_add_merge()
> > >>>>>>>>>>>> function.  I'll cook up another patch if it's the preferred way.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> Yeah lets just use a pointer and see how that works out.  Thanks,
> > >>>>>>>>>>
> > >>>>>>>>>> Oops, I forgot that ulist_add() takes aux as u64 and it calling
> > >>>>>>>>>> ulist_add_merge() internally.  So, we can't change the type blindly
> > >>>>>>>>>> there, unfortunately.
> > >>>>>>>>>
> > >>>>>>>>> Looking back at the code, it seems that all aux arguments passed to
> > >>>>>>>>> ulist_add() in qgroup.c are pointers, too.  So, indeed, all aux values
> > >>>>>>>>> are pointers, so far, and it'd be even cleaner to replace all these
> > >>>>>>>>> from u64 to void *.
> > >>>>>>>>>
> > >>>>>>>>> But, such a replacement patch will become difficult for backporting to
> > >>>>>>>>> stable kernels (the bug existed since 3.11, at least).  So IMO, we
> > >>>>>>>>> should put a smaller fix like my previous one, let it backported to
> > >>>>>>>>> stable kernels, and do more comprehensive replacements to pointer on
> > >>>>>>>>> its top.
> > >>>>>>>>
> > >>>>>>>> Ping.  Could you guys take my original patch as is, or do you prefer
> > >>>>>>>> changing in a different way?  If so, how?
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> I don't care how hard it is to backport to stable,
> > >>>>>>
> > >>>>>> You must do care as a maintainer.  It's a long-standing and serious
> > >>>>>> bug since 3.11.  The kernel hangs up immediately when you enable quota
> > >>>>>> on 32bit kernel.  And it's really hard to revert it when the rootfs is
> > >>>>>> btrfs.  (The mount follows the immediate hang up after reboot.)
> > >>>>>>
> > >>>>
> > >>>> What I mean is that we want the right fix first, not something that is easier to
> > >>>> pull back to stable and then the right fix later.  Do it right first and then
> > >>>> backport it to the stable kernels, it's perfectly acceptable to adjust patches
> > >>>> when sending them to the stable team.  "But it's hard" is not a valid excuse for
> > >>>> not doing it right the first time.
> > >>>
> > >>> Well, I guess this underestimates the burden of backports.  Currently,
> > >>> there is stable kernel for each kernel release.  Anyone has to
> > >>> backport for each version, and you'll be asked.  I, as a long-time
> > >>> subsystem maintainer, wouldn't go in that way :)
> > >>>
> > >>> And, speaking of "rightness" -- replacing the callers with a wrapper
> > >>> is also a right fix.  It's even a safer fix.  That's basically why I
> > >>> posted it as the primary patch.
> > >>>
> > >>> The merit of replacing all callers is that you can eliminate nasty
> > >>> casts by that.  This is however rather a cleanup, which is a different
> > >>> bonus from what we need to fix.
> > >>>
> > >>>>>>> since we're using pointers
> > >>>>>>> everywhere just change it to void * and be done with it.  Thanks,
> > >>>>>>
> > >>>>>> Fix it quickly, then do cleanup.  This is the golden rule for
> > >>>>>> regression :)
> > >>>>>
> > >>>>> Also, another question is whether you guys are OK to change the type
> > >>>>> to a pointer.  Through a glance, the ulist code was intended to handle
> > >>>>> any generic data, thus it uses u64, right?  Using void pointer breaks
> > >>>>> this concept.
> > >>>>>
> > >>>>
> > >>>> It's fine, ulist today resembles very little from what it was originally.  The
> > >>>> current users all shove pointers into there, so we might as well just make it a
> > >>>> pointer.  Thanks,
> > >>>
> > >>> OK, good to know.
> > >>>
> > >>> If this post still doesn't convince you, I'll prepare the patch to do
> > >>> all replacements.
> > >>>
> > >>>
> > >>
> > >> I don't care that much, do it however you want.  Thanks,
> > >
> > > Yes, I do care because I know of this kind of horror very well.
> > >
> > > OK, I'm going to send the new patch(es).
> > >
> > 
> > I meant that I don't care how you do it, if you want to do it the simple way
> > first and then send the cleanup later thats fine by me.  Thanks,
> 
> Ah, misunderstood.  Then please disregard my v2 patches and take just
> v1 patch.  It's even good for 3.16, if possible.
> 
> I'll send the cleanup patch on top of that.

Ping.  I don't want to play a bot, but slowly wondering what's going
on...


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chris Mason Aug. 12, 2014, 5:39 p.m. UTC | #17

On 08/06/2014 11:14 AM, Takashi Iwai wrote:
>>>
>>> I meant that I don't care how you do it, if you want to do it the simple way
>>> first and then send the cleanup later thats fine by me.  Thanks,
>>
>> Ah, misunderstood.  Then please disregard my v2 patches and take just
>> v1 patch.  It's even good for 3.16, if possible.
>>
>> I'll send the cleanup patch on top of that.
> 
> Ping.  I don't want to play a bot, but slowly wondering what's going
> on...

Sorry for the delay Takashi.  This patch is a ';' away from compiling.
I've fixed it up here and I'm running tests, but I wanted to double
check this was the latest version.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Takashi Iwai Aug. 14, 2014, 9:54 a.m. UTC | #18

At Tue, 12 Aug 2014 13:39:11 -0400,
Chris Mason wrote:
> 
> 
> 
> On 08/06/2014 11:14 AM, Takashi Iwai wrote:
> >>>
> >>> I meant that I don't care how you do it, if you want to do it the simple way
> >>> first and then send the cleanup later thats fine by me.  Thanks,
> >>
> >> Ah, misunderstood.  Then please disregard my v2 patches and take just
> >> v1 patch.  It's even good for 3.16, if possible.
> >>
> >> I'll send the cleanup patch on top of that.
> > 
> > Ping.  I don't want to play a bot, but slowly wondering what's going
> > on...
> 
> Sorry for the delay Takashi.  This patch is a ';' away from compiling.
> I've fixed it up here and I'm running tests, but I wanted to double
> check this was the latest version.

Yes, the first version (posted on July 28) is the right patch.
(Sorry for the missing ";", I thought I did build test on both
 architectures.)


thanks,

Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index e25564bfcb46..d7a24620a963 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -276,9 +276,8 @@  static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path,
 			}
 			if (ret > 0)
 				goto next;
-			ret = ulist_add_merge(parents, eb->start,
-					      (uintptr_t)eie,
-					      (u64 *)&old, GFP_NOFS);
+			ret = ulist_add_merge_ptr(parents, eb->start,
+						  eie, (void **)&old, GFP_NOFS);
 			if (ret < 0)
 				break;
 			if (!ret && extent_item_pos) {
@@ -1008,9 +1007,9 @@  again:
 					goto out;
 				ref->inode_list = eie;
 			}
-			ret = ulist_add_merge(refs, ref->parent,
-					      (uintptr_t)ref->inode_list,
-					      (u64 *)&eie, GFP_NOFS);
+			ret = ulist_add_merge_ptr(refs, ref->parent,
+						  ref->inode_list,
+						  (void **)&eie, GFP_NOFS);
 			if (ret < 0)
 				goto out;
 			if (!ret && extent_item_pos) {
diff --git a/fs/btrfs/ulist.h b/fs/btrfs/ulist.h
index 7f78cbf5cf41..695fc2bac680 100644
--- a/fs/btrfs/ulist.h
+++ b/fs/btrfs/ulist.h
@@ -57,6 +57,21 @@  void ulist_free(struct ulist *ulist);
 int ulist_add(struct ulist *ulist, u64 val, u64 aux, gfp_t gfp_mask);
 int ulist_add_merge(struct ulist *ulist, u64 val, u64 aux,
 		    u64 *old_aux, gfp_t gfp_mask);
+
+/* just like ulist_add_merge() but take a pointer for the aux data */
+static inline int ulist_add_merge_ptr(struct ulist *ulist, u64 val, void *aux,
+				      void **old_aux, gfp_t gfp_mask)
+{
+#if BITS_PER_LONG == 32
+	u64 old64 = (uintptr_t)*old_aux;
+	int ret = ulist_add_merge(ulist, val, (uintptr_t)aux, &old64, gfp_mask);
+	*old_aux = (void *)((uintptr_t)old64);
+	return ret;
+#else
+	return ulist_add_merge(ulist, val, (u64)aux, (u64 *)old_aux, gfp_mask)
+#endif
+}
+
 struct ulist_node *ulist_next(struct ulist *ulist,
 			      struct ulist_iterator *uiter);