Prev: hmc6352: Add driver for the HMC6352 compass
Next: Eliminate duplicated timer code
From: Jiri Olsa on 19 Jun 2010 08:10
On Fri, Jun 18, 2010 at 12:50:40AM +0100, David Howells wrote:
> Jiri Olsa <jolsa(a)redhat.com> wrote:
>
> > maybe I have better solution...
> >
> > I think there's no need to get the cred refference as long as
> > the 'cred' handling stays inside the rcu_read_lock block.
>
> I think this is right. There should be no need for a synchronize_rcu() call
> to be added in commit_creds() with this as commit_creds() calls put_cred()
> which will defer the destruction until the RCU grace period is up anyway.
>
> Whilst I'd prefer to call get_cred() in task_state(), as you point out (and I
> hadn't considered), this may see an cred struct that has been detached from
> its pointer on another CPU and had its usage count reduced to 0.
>
> In such a case, we can't simply increment the count and then decrement it
> again later as it's already on the RCU destruction queue and can't necessarily
> be removed so that it can be added back in.
>
> What could be done, though I'm not sure it's worth it, is to use
> atomic_inc_not_zero() and loop around if the cred struct has gone out of
> service when we try and access it and reread the pointer.
>
> The advantage of this would be that we could manage to hold the RCU read lock
> for as little time as possible.
>
> Acked-by: David Howells <dhowells(a)redhat.com>

thanks,

in case the there's need for some consistent comment :)
patch stays

wbr,
jirka



---
BZ 591015 - kernel BUG at kernel/cred.c:168
https://bugzilla.redhat.com/show_bug.cgi?id=591015

Above bugzilla reported bug during the releasing of
old cred structure.

There is reproducer attached to the bugzilla.

The issue is caused by releasing old cred struct while other
kernel path might be still using it. This leads to cred->usage
inconsistency inside the __put_cred and triggering the bug.

Following kernel paths are affected:

The CPU1 path is setting the new groups creds.
The CPU2 path is cat /proc/PID/status


CPU 1 CPU 2

sys_setgroups proc_pid_status
set_current_groups task_state
commit_creds rcu_read_lock
put_cred ...
__put_cred get_cred
BUG_ON(usage != 0) ...
rcu_read_unlock



If __put_cred got executed during the CPU2 holding the reference
the BUG_ON inside __put_cred is trigered.

I think there's no need to get the cred refference as long as
the 'cred' handling stays inside the rcu_read_lock block.

And the condition of __task_cred 'make sure task doesn't go away',
is done by proc_single_show as this is the proc file.

wbr,
jirka


Signed-off-by: Jiri Olsa <jolsa(a)redhat.com>
---
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 9b58d38..ac3b3a4 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -176,7 +176,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
if (tracer)
tpid = task_pid_nr_ns(tracer, ns);
}
- cred = get_cred((struct cred *) __task_cred(p));
+ cred = __task_cred(p);
seq_printf(m,
"State:\t%s\n"
"Tgid:\t%d\n"
@@ -199,15 +199,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
"FDSize:\t%d\n"
"Groups:\t",
fdt ? fdt->max_fds : 0);
- rcu_read_unlock();

group_info = cred->group_info;
task_unlock(p);

for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
seq_printf(m, "%d ", GROUP_AT(group_info, g));
- put_cred(cred);

+ rcu_read_unlock();
seq_printf(m, "\n");
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Olsa on 25 Jun 2010 09:00
hi,

do I need to send this some one/place else?
not sure it's going to be picked up..

thanks,
jirka

On Sat, Jun 19, 2010 at 02:01:02PM +0200, Jiri Olsa wrote:
> On Fri, Jun 18, 2010 at 12:50:40AM +0100, David Howells wrote:
> > Jiri Olsa <jolsa(a)redhat.com> wrote:
> >
> > > maybe I have better solution...
> > >
> > > I think there's no need to get the cred refference as long as
> > > the 'cred' handling stays inside the rcu_read_lock block.
> >
> > I think this is right. There should be no need for a synchronize_rcu() call
> > to be added in commit_creds() with this as commit_creds() calls put_cred()
> > which will defer the destruction until the RCU grace period is up anyway.
> >
> > Whilst I'd prefer to call get_cred() in task_state(), as you point out (and I
> > hadn't considered), this may see an cred struct that has been detached from
> > its pointer on another CPU and had its usage count reduced to 0.
> >
> > In such a case, we can't simply increment the count and then decrement it
> > again later as it's already on the RCU destruction queue and can't necessarily
> > be removed so that it can be added back in.
> >
> > What could be done, though I'm not sure it's worth it, is to use
> > atomic_inc_not_zero() and loop around if the cred struct has gone out of
> > service when we try and access it and reread the pointer.
> >
> > The advantage of this would be that we could manage to hold the RCU read lock
> > for as little time as possible.
> >
> > Acked-by: David Howells <dhowells(a)redhat.com>
>
> thanks,
>
> in case the there's need for some consistent comment :)
> patch stays
>
> wbr,
> jirka
>
>
>
> ---
> BZ 591015 - kernel BUG at kernel/cred.c:168
> https://bugzilla.redhat.com/show_bug.cgi?id=591015
>
> Above bugzilla reported bug during the releasing of
> old cred structure.
>
> There is reproducer attached to the bugzilla.
>
> The issue is caused by releasing old cred struct while other
> kernel path might be still using it. This leads to cred->usage
> inconsistency inside the __put_cred and triggering the bug.
>
> Following kernel paths are affected:
>
> The CPU1 path is setting the new groups creds.
> The CPU2 path is cat /proc/PID/status
>
>
> CPU 1 CPU 2
>
> sys_setgroups proc_pid_status
> set_current_groups task_state
> commit_creds rcu_read_lock
> put_cred ...
> __put_cred get_cred
> BUG_ON(usage != 0) ...
> rcu_read_unlock
>
>
>
> If __put_cred got executed during the CPU2 holding the reference
> the BUG_ON inside __put_cred is trigered.
>
> I think there's no need to get the cred refference as long as
> the 'cred' handling stays inside the rcu_read_lock block.
>
> And the condition of __task_cred 'make sure task doesn't go away',
> is done by proc_single_show as this is the proc file.
>
> wbr,
> jirka
>
>
> Signed-off-by: Jiri Olsa <jolsa(a)redhat.com>
> ---
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 9b58d38..ac3b3a4 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -176,7 +176,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> if (tracer)
> tpid = task_pid_nr_ns(tracer, ns);
> }
> - cred = get_cred((struct cred *) __task_cred(p));
> + cred = __task_cred(p);
> seq_printf(m,
> "State:\t%s\n"
> "Tgid:\t%d\n"
> @@ -199,15 +199,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> "FDSize:\t%d\n"
> "Groups:\t",
> fdt ? fdt->max_fds : 0);
> - rcu_read_unlock();
>
> group_info = cred->group_info;
> task_unlock(p);
>
> for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
> seq_printf(m, "%d ", GROUP_AT(group_info, g));
> - put_cred(cred);
>
> + rcu_read_unlock();
> seq_printf(m, "\n");
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 25 Jun 2010 09:30
Jiri Olsa <jolsa(a)redhat.com> wrote:

> do I need to send this some one/place else?
> not sure it's going to be picked up..

Post it to linux-security-module(a)vger.kernel.org with the ACKs you've
collected added in.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Olsa on 2 Jul 2010 08:20
hi,
any feedback?

jirka

On Fri, Jun 25, 2010 at 09:33:33AM -0400, Jiri Olsa wrote:
> BZ 591015 - kernel BUG at kernel/cred.c:168
> https://bugzilla.redhat.com/show_bug.cgi?id=591015
>
> Above bugzilla reported bug during the releasing of
> old cred structure.
>
> There is reproducer attached to the bugzilla.
>
> The issue is caused by releasing old cred struct while other
> kernel path might be still using it. This leads to cred->usage
> inconsistency inside the __put_cred and triggering the bug.
>
> Following kernel paths are affected:
>
> The CPU1 path is setting the new groups creds.
> The CPU2 path is cat /proc/PID/status
>
>
> CPU 1 CPU 2
>
> sys_setgroups proc_pid_status
> set_current_groups task_state
> commit_creds rcu_read_lock
> put_cred ...
> __put_cred get_cred
> BUG_ON(usage != 0) ...
> rcu_read_unlock
>
>
>
> If __put_cred got executed during the CPU2 holding the reference
> the BUG_ON inside __put_cred is trigered.
>
> I think there's no need to get the cred refference as long as
> the 'cred' handling stays inside the rcu_read_lock block.
>
> And the condition of __task_cred 'make sure task doesn't go away',
> is done by proc_single_show as this is the proc file.
>
> wbr,
> jirka
>
>
> Signed-off-by: Jiri Olsa <jolsa(a)redhat.com>
> Acked-by: David Howells <dhowells(a)redhat.com>
> ---
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 9b58d38..ac3b3a4 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -176,7 +176,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> if (tracer)
> tpid = task_pid_nr_ns(tracer, ns);
> }
> - cred = get_cred((struct cred *) __task_cred(p));
> + cred = __task_cred(p);
> seq_printf(m,
> "State:\t%s\n"
> "Tgid:\t%d\n"
> @@ -199,15 +199,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
> "FDSize:\t%d\n"
> "Groups:\t",
> fdt ? fdt->max_fds : 0);
> - rcu_read_unlock();
>
> group_info = cred->group_info;
> task_unlock(p);
>
> for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
> seq_printf(m, "%d ", GROUP_AT(group_info, g));
> - put_cred(cred);
>
> + rcu_read_unlock();
> seq_printf(m, "\n");
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 27 Jul 2010 12:20
On Tue, Jul 27, 2010 at 8:50 AM, Jiri Olsa <jolsa(a)redhat.com> wrote:
>
> got no objections on linux-security-module and acked by David.
> Noone pick it, so got advice to send this directly to you.

The patch seems fundamentally buggy.

The whole patch seems to be based on "nobody can ever use
get_cred/put_cred, because concurrent use will then trigger the
BUG_ON() in __put_cred()".

But that's a bug in general, not in this particular usage that isn't
all that different from other uses. So rather than just remove the
code that uses the refcounting, we should either:

- FIX the damn ref-counting so that it works without bugging out

or

- remove the broken functions entirely.

In other words - why are we working around what looks like a bug,
rather than fixing the bug itself?

In particular, the code you remove seems to be basically _identical_
to get_task_cred(). So if the code you remove is buggy, then so is any
use of get_task_cred() - no?

So please explain why get_task_cred() is ok, but the particular use of
get_cred/put_cred that you removed is not.

Hmm? What am I missing?

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: hmc6352: Add driver for the HMC6352 compass
Next: Eliminate duplicated timer code