task_struct::cred is only used task-synchronously and does
not require any RCU locks, hence, rcu_dereference_check is
not required to read from it.
Suggested-by: Jann Horn <[email protected]>
Co-developed-by: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Joel Fernandes (Google) <[email protected]>
Signed-off-by: Amol Grover <[email protected]>
---
kernel/auditsc.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4effe01ebbe2..d3510513cdd1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
/* Determine if any context name data matches a rule's watch data */
/* Compare a task_struct with an audit_rule. Return 1 on match, 0
* otherwise.
- *
- * If task_creation is true, this is an explicit indication that we are
- * filtering a task rule at task creation time. This and tsk == current are
- * the only situations where tsk->cred may be accessed without an rcu read lock.
*/
static int audit_filter_rules(struct task_struct *tsk,
struct audit_krule *rule,
struct audit_context *ctx,
struct audit_names *name,
- enum audit_state *state,
- bool task_creation)
+ enum audit_state *state)
{
const struct cred *cred;
int i, need_sid = 1;
u32 sid;
unsigned int sessionid;
- cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
+ cred = tsk->cred;
for (i = 0; i < rule->field_count; i++) {
struct audit_field *f = &rule->fields[i];
@@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
rcu_read_lock();
list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
- &state, true)) {
+ &state)) {
if (state == AUDIT_RECORD_CONTEXT)
*key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
rcu_read_unlock();
@@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
list_for_each_entry_rcu(e, list, list) {
if (audit_in_mask(&e->rule, ctx->major) &&
audit_filter_rules(tsk, &e->rule, ctx, NULL,
- &state, false)) {
+ &state)) {
rcu_read_unlock();
ctx->current_state = state;
return state;
@@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
list_for_each_entry_rcu(e, list, list) {
if (audit_in_mask(&e->rule, ctx->major) &&
- audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
+ audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
ctx->current_state = state;
return 1;
}
--
2.24.1
On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> task_struct::cred is only used task-synchronously and does
> not require any RCU locks, hence, rcu_dereference_check is
> not required to read from it.
>
> Suggested-by: Jann Horn <[email protected]>
> Co-developed-by: Joel Fernandes (Google) <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> Signed-off-by: Amol Grover <[email protected]>
> ---
> kernel/auditsc.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
This is the exact same patch I ACK'd back in February, yes?
https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4effe01ebbe2..d3510513cdd1 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
> /* Determine if any context name data matches a rule's watch data */
> /* Compare a task_struct with an audit_rule. Return 1 on match, 0
> * otherwise.
> - *
> - * If task_creation is true, this is an explicit indication that we are
> - * filtering a task rule at task creation time. This and tsk == current are
> - * the only situations where tsk->cred may be accessed without an rcu read lock.
> */
> static int audit_filter_rules(struct task_struct *tsk,
> struct audit_krule *rule,
> struct audit_context *ctx,
> struct audit_names *name,
> - enum audit_state *state,
> - bool task_creation)
> + enum audit_state *state)
> {
> const struct cred *cred;
> int i, need_sid = 1;
> u32 sid;
> unsigned int sessionid;
>
> - cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
> + cred = tsk->cred;
>
> for (i = 0; i < rule->field_count; i++) {
> struct audit_field *f = &rule->fields[i];
> @@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
> rcu_read_lock();
> list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
> if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
> - &state, true)) {
> + &state)) {
> if (state == AUDIT_RECORD_CONTEXT)
> *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
> rcu_read_unlock();
> @@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
> list_for_each_entry_rcu(e, list, list) {
> if (audit_in_mask(&e->rule, ctx->major) &&
> audit_filter_rules(tsk, &e->rule, ctx, NULL,
> - &state, false)) {
> + &state)) {
> rcu_read_unlock();
> ctx->current_state = state;
> return state;
> @@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
>
> list_for_each_entry_rcu(e, list, list) {
> if (audit_in_mask(&e->rule, ctx->major) &&
> - audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
> + audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
> ctx->current_state = state;
> return 1;
> }
> --
> 2.24.1
--
paul moore
http://www.paul-moore.com
On Thu, Apr 02, 2020 at 08:56:36AM -0400, Paul Moore wrote:
> On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> > task_struct::cred is only used task-synchronously and does
> > not require any RCU locks, hence, rcu_dereference_check is
> > not required to read from it.
> >
> > Suggested-by: Jann Horn <[email protected]>
> > Co-developed-by: Joel Fernandes (Google) <[email protected]>
> > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > Signed-off-by: Amol Grover <[email protected]>
> > ---
> > kernel/auditsc.c | 15 +++++----------
> > 1 file changed, 5 insertions(+), 10 deletions(-)
>
> This is the exact same patch I ACK'd back in February, yes?
>
> https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
>
Hi Paul,
That's correct. I've resend the series out of the fear that the first 2
patches might've gotten lost as it's been almost a month since I last
sent them. Could you please ack this again, and if you don't mind could
you please go through the other 2 patches and ack them aswell?
Thanks
Amol
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 4effe01ebbe2..d3510513cdd1 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
> > /* Determine if any context name data matches a rule's watch data */
> > /* Compare a task_struct with an audit_rule. Return 1 on match, 0
> > * otherwise.
> > - *
> > - * If task_creation is true, this is an explicit indication that we are
> > - * filtering a task rule at task creation time. This and tsk == current are
> > - * the only situations where tsk->cred may be accessed without an rcu read lock.
> > */
> > static int audit_filter_rules(struct task_struct *tsk,
> > struct audit_krule *rule,
> > struct audit_context *ctx,
> > struct audit_names *name,
> > - enum audit_state *state,
> > - bool task_creation)
> > + enum audit_state *state)
> > {
> > const struct cred *cred;
> > int i, need_sid = 1;
> > u32 sid;
> > unsigned int sessionid;
> >
> > - cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
> > + cred = tsk->cred;
> >
> > for (i = 0; i < rule->field_count; i++) {
> > struct audit_field *f = &rule->fields[i];
> > @@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
> > rcu_read_lock();
> > list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
> > if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
> > - &state, true)) {
> > + &state)) {
> > if (state == AUDIT_RECORD_CONTEXT)
> > *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
> > rcu_read_unlock();
> > @@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
> > list_for_each_entry_rcu(e, list, list) {
> > if (audit_in_mask(&e->rule, ctx->major) &&
> > audit_filter_rules(tsk, &e->rule, ctx, NULL,
> > - &state, false)) {
> > + &state)) {
> > rcu_read_unlock();
> > ctx->current_state = state;
> > return state;
> > @@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
> >
> > list_for_each_entry_rcu(e, list, list) {
> > if (audit_in_mask(&e->rule, ctx->major) &&
> > - audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
> > + audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
> > ctx->current_state = state;
> > return 1;
> > }
> > --
> > 2.24.1
>
> --
> paul moore
> http://www.paul-moore.com
On Fri, Apr 3, 2020 at 3:56 AM Amol Grover <[email protected]> wrote:
> On Thu, Apr 02, 2020 at 08:56:36AM -0400, Paul Moore wrote:
> > On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> > > task_struct::cred is only used task-synchronously and does
> > > not require any RCU locks, hence, rcu_dereference_check is
> > > not required to read from it.
> > >
> > > Suggested-by: Jann Horn <[email protected]>
> > > Co-developed-by: Joel Fernandes (Google) <[email protected]>
> > > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > > Signed-off-by: Amol Grover <[email protected]>
> > > ---
> > > kernel/auditsc.c | 15 +++++----------
> > > 1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > This is the exact same patch I ACK'd back in February, yes?
> >
> > https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
> >
>
> Hi Paul,
>
> That's correct. I've resend the series out of the fear that the first 2
> patches might've gotten lost as it's been almost a month since I last
> sent them. Could you please ack this again, and if you don't mind could
> you please go through the other 2 patches and ack them aswell?
If you hadn't changed the patch at all, and it doesn't look like you
did, you could have (and likely should have) just carried over my ACK.
Regardless, I'll re-ACK it now (below). As far as the other two
patches are concerned, they look okay to me but I would defer my ACK
to maintainer of that code.
Acked-by: Paul Moore <[email protected]>
--
paul moore
http://www.paul-moore.com
On 2020-04-03 13:26, Amol Grover wrote:
> On Thu, Apr 02, 2020 at 08:56:36AM -0400, Paul Moore wrote:
> > On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> > > task_struct::cred is only used task-synchronously and does
> > > not require any RCU locks, hence, rcu_dereference_check is
> > > not required to read from it.
> > >
> > > Suggested-by: Jann Horn <[email protected]>
> > > Co-developed-by: Joel Fernandes (Google) <[email protected]>
> > > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > > Signed-off-by: Amol Grover <[email protected]>
> > > ---
> > > kernel/auditsc.c | 15 +++++----------
> > > 1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > This is the exact same patch I ACK'd back in February, yes?
> >
> > https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
> >
>
> Hi Paul,
>
> That's correct. I've resend the series out of the fear that the first 2
> patches might've gotten lost as it's been almost a month since I last
> sent them. Could you please ack this again, and if you don't mind could
> you please go through the other 2 patches and ack them aswell?
Via who's tree are you expecting this will make it upstream?
> Thanks
> Amol
>
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 4effe01ebbe2..d3510513cdd1 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -430,24 +430,19 @@ static int audit_field_compare(struct task_struct *tsk,
> > > /* Determine if any context name data matches a rule's watch data */
> > > /* Compare a task_struct with an audit_rule. Return 1 on match, 0
> > > * otherwise.
> > > - *
> > > - * If task_creation is true, this is an explicit indication that we are
> > > - * filtering a task rule at task creation time. This and tsk == current are
> > > - * the only situations where tsk->cred may be accessed without an rcu read lock.
> > > */
> > > static int audit_filter_rules(struct task_struct *tsk,
> > > struct audit_krule *rule,
> > > struct audit_context *ctx,
> > > struct audit_names *name,
> > > - enum audit_state *state,
> > > - bool task_creation)
> > > + enum audit_state *state)
> > > {
> > > const struct cred *cred;
> > > int i, need_sid = 1;
> > > u32 sid;
> > > unsigned int sessionid;
> > >
> > > - cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
> > > + cred = tsk->cred;
> > >
> > > for (i = 0; i < rule->field_count; i++) {
> > > struct audit_field *f = &rule->fields[i];
> > > @@ -745,7 +740,7 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
> > > rcu_read_lock();
> > > list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
> > > if (audit_filter_rules(tsk, &e->rule, NULL, NULL,
> > > - &state, true)) {
> > > + &state)) {
> > > if (state == AUDIT_RECORD_CONTEXT)
> > > *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
> > > rcu_read_unlock();
> > > @@ -791,7 +786,7 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
> > > list_for_each_entry_rcu(e, list, list) {
> > > if (audit_in_mask(&e->rule, ctx->major) &&
> > > audit_filter_rules(tsk, &e->rule, ctx, NULL,
> > > - &state, false)) {
> > > + &state)) {
> > > rcu_read_unlock();
> > > ctx->current_state = state;
> > > return state;
> > > @@ -815,7 +810,7 @@ static int audit_filter_inode_name(struct task_struct *tsk,
> > >
> > > list_for_each_entry_rcu(e, list, list) {
> > > if (audit_in_mask(&e->rule, ctx->major) &&
> > > - audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
> > > + audit_filter_rules(tsk, &e->rule, ctx, n, &state)) {
> > > ctx->current_state = state;
> > > return 1;
> > > }
> > > --
> > > 2.24.1
> >
> > paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
On Fri, Apr 3, 2020 at 5:22 PM Richard Guy Briggs <[email protected]> wrote:
> On 2020-04-03 13:26, Amol Grover wrote:
> > On Thu, Apr 02, 2020 at 08:56:36AM -0400, Paul Moore wrote:
> > > On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> > > > task_struct::cred is only used task-synchronously and does
> > > > not require any RCU locks, hence, rcu_dereference_check is
> > > > not required to read from it.
> > > >
> > > > Suggested-by: Jann Horn <[email protected]>
> > > > Co-developed-by: Joel Fernandes (Google) <[email protected]>
> > > > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > > > Signed-off-by: Amol Grover <[email protected]>
> > > > ---
> > > > kernel/auditsc.c | 15 +++++----------
> > > > 1 file changed, 5 insertions(+), 10 deletions(-)
> > >
> > > This is the exact same patch I ACK'd back in February, yes?
> > >
> > > https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
> > >
> >
> > Hi Paul,
> >
> > That's correct. I've resend the series out of the fear that the first 2
> > patches might've gotten lost as it's been almost a month since I last
> > sent them. Could you please ack this again, and if you don't mind could
> > you please go through the other 2 patches and ack them aswell?
>
> Via who's tree are you expecting this will make it upstream?
When I asked a similar question back in February the response was
basically not the audit tree.
--
paul moore
http://www.paul-moore.com
On 2020-04-03 17:43, Paul Moore wrote:
> On Fri, Apr 3, 2020 at 5:22 PM Richard Guy Briggs <[email protected]> wrote:
> > On 2020-04-03 13:26, Amol Grover wrote:
> > > On Thu, Apr 02, 2020 at 08:56:36AM -0400, Paul Moore wrote:
> > > > On Thu, Apr 2, 2020 at 1:57 AM Amol Grover <[email protected]> wrote:
> > > > > task_struct::cred is only used task-synchronously and does
> > > > > not require any RCU locks, hence, rcu_dereference_check is
> > > > > not required to read from it.
> > > > >
> > > > > Suggested-by: Jann Horn <[email protected]>
> > > > > Co-developed-by: Joel Fernandes (Google) <[email protected]>
> > > > > Signed-off-by: Joel Fernandes (Google) <[email protected]>
> > > > > Signed-off-by: Amol Grover <[email protected]>
> > > > > ---
> > > > > kernel/auditsc.c | 15 +++++----------
> > > > > 1 file changed, 5 insertions(+), 10 deletions(-)
> > > >
> > > > This is the exact same patch I ACK'd back in February, yes?
> > > >
> > > > https://lore.kernel.org/linux-audit/CAHC9VhQCbg1V290bYEZM+izDPRpr=XYXakohnDaMphkBBFgUaA@mail.gmail.com
> > > >
> > >
> > > Hi Paul,
> > >
> > > That's correct. I've resend the series out of the fear that the first 2
> > > patches might've gotten lost as it's been almost a month since I last
> > > sent them. Could you please ack this again, and if you don't mind could
> > > you please go through the other 2 patches and ack them aswell?
> >
> > Via who's tree are you expecting this will make it upstream?
>
> When I asked a similar question back in February the response was
> basically not the audit tree.
Well, I went checking mingo and akpm's trees and didn't find 1/3 and 2/3
there even though I thought 3/3 was in audit/stable-5.6. I was mistaken,
that patch in audit/stable-5.6 is a previous rcu fix for auditd_conn and
not 3/3.
> paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635