Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Fri, Apr 20, 2018 at 1:48 PM, Richard Guy Briggswrote: > On 2018-04-20 11:58, Paul Moore wrote: >> On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggs wrote: >> > On 2018-04-17 18:06, Paul Moore wrote: >> >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs >> >> wrote: >> >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> >> > user action. >> >> > >> >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> >> > >> >> > Signed-off-by: Richard Guy Briggs >> >> > --- >> >> > kernel/audit.c | 5 ++--- >> >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> >> > >> >> > diff --git a/kernel/audit.c b/kernel/audit.c >> >> > index 8da24ef..23f125b 100644 >> >> > --- a/kernel/audit.c >> >> > +++ b/kernel/audit.c >> >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, >> >> > u32 old_feature, u32 new_feature >> >> > { >> >> > struct audit_buffer *ab; >> >> > >> >> > - if (audit_enabled == AUDIT_OFF) >> >> > + if (!audit_enabled) >> >> >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> >> why are you adding noise to this patch? >> > >> > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 >> > instances where it could be used as a boolean where the expression is >> > made harder to read (in my opinion). I thought it was worth changing to >> > read the same way most of the other instances I've been reviewing are >> > written. There are only two where the non-boolean distiction with >> > AUDIT_LOCKED is required. >> >> Thanks for the explanation. >> >> While I still believe this patch, and connecting records in general, >> is the Right Thing To Do, I'm expecting there to be some hate mail on >> this issue and I would like to keep the patch as small and as >> straight-to-the-point as possible just so there is little confusion >> about what is changing. >> >> Please respin this without the style change and I'll merge it as soon >> as I see it. Alternatively, give me the "ok" and I'll merge the patch >> now and just drop the style change; after all we're talking about one >> line in a five line patch ;) > > Go ahead and drop that style change line to simplify this patch and I'll > submit another patch to clean them all up at the same time (probably the > next time one of those changes). Sounds good. Merged. Thanks Richard. -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Fri, Apr 20, 2018 at 1:48 PM, Richard Guy Briggs wrote: > On 2018-04-20 11:58, Paul Moore wrote: >> On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggs wrote: >> > On 2018-04-17 18:06, Paul Moore wrote: >> >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs >> >> wrote: >> >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> >> > user action. >> >> > >> >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> >> > >> >> > Signed-off-by: Richard Guy Briggs >> >> > --- >> >> > kernel/audit.c | 5 ++--- >> >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> >> > >> >> > diff --git a/kernel/audit.c b/kernel/audit.c >> >> > index 8da24ef..23f125b 100644 >> >> > --- a/kernel/audit.c >> >> > +++ b/kernel/audit.c >> >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, >> >> > u32 old_feature, u32 new_feature >> >> > { >> >> > struct audit_buffer *ab; >> >> > >> >> > - if (audit_enabled == AUDIT_OFF) >> >> > + if (!audit_enabled) >> >> >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> >> why are you adding noise to this patch? >> > >> > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 >> > instances where it could be used as a boolean where the expression is >> > made harder to read (in my opinion). I thought it was worth changing to >> > read the same way most of the other instances I've been reviewing are >> > written. There are only two where the non-boolean distiction with >> > AUDIT_LOCKED is required. >> >> Thanks for the explanation. >> >> While I still believe this patch, and connecting records in general, >> is the Right Thing To Do, I'm expecting there to be some hate mail on >> this issue and I would like to keep the patch as small and as >> straight-to-the-point as possible just so there is little confusion >> about what is changing. >> >> Please respin this without the style change and I'll merge it as soon >> as I see it. Alternatively, give me the "ok" and I'll merge the patch >> now and just drop the style change; after all we're talking about one >> line in a five line patch ;) > > Go ahead and drop that style change line to simplify this patch and I'll > submit another patch to clean them all up at the same time (probably the > next time one of those changes). Sounds good. Merged. Thanks Richard. -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On 2018-04-20 11:58, Paul Moore wrote: > On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggswrote: > > On 2018-04-17 18:06, Paul Moore wrote: > >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs > >> wrote: > >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of > >> > user action. > >> > > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 > >> > > >> > Signed-off-by: Richard Guy Briggs > >> > --- > >> > kernel/audit.c | 5 ++--- > >> > 1 file changed, 2 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/kernel/audit.c b/kernel/audit.c > >> > index 8da24ef..23f125b 100644 > >> > --- a/kernel/audit.c > >> > +++ b/kernel/audit.c > >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, > >> > u32 old_feature, u32 new_feature > >> > { > >> > struct audit_buffer *ab; > >> > > >> > - if (audit_enabled == AUDIT_OFF) > >> > + if (!audit_enabled) > >> > >> Sooo, this is an unrelated style change, why? Looking at the rest of > >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > >> why are you adding noise to this patch? > > > > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 > > instances where it could be used as a boolean where the expression is > > made harder to read (in my opinion). I thought it was worth changing to > > read the same way most of the other instances I've been reviewing are > > written. There are only two where the non-boolean distiction with > > AUDIT_LOCKED is required. > > Thanks for the explanation. > > While I still believe this patch, and connecting records in general, > is the Right Thing To Do, I'm expecting there to be some hate mail on > this issue and I would like to keep the patch as small and as > straight-to-the-point as possible just so there is little confusion > about what is changing. > > Please respin this without the style change and I'll merge it as soon > as I see it. Alternatively, give me the "ok" and I'll merge the patch > now and just drop the style change; after all we're talking about one > line in a five line patch ;) Go ahead and drop that style change line to simplify this patch and I'll submit another patch to clean them all up at the same time (probably the next time one of those changes). > >> > return; > >> > - > >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > >> > AUDIT_FEATURE_CHANGE); > >> > >> This is the important part, and the Right Thing To Do. > >> > >> > if (!ab) > >> > return; > >> > audit_log_task_info(ab, current); > > -- > paul moore > www.paul-moore.com - RGB -- Richard Guy Briggs 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
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On 2018-04-20 11:58, Paul Moore wrote: > On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggs wrote: > > On 2018-04-17 18:06, Paul Moore wrote: > >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs > >> wrote: > >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of > >> > user action. > >> > > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 > >> > > >> > Signed-off-by: Richard Guy Briggs > >> > --- > >> > kernel/audit.c | 5 ++--- > >> > 1 file changed, 2 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/kernel/audit.c b/kernel/audit.c > >> > index 8da24ef..23f125b 100644 > >> > --- a/kernel/audit.c > >> > +++ b/kernel/audit.c > >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, > >> > u32 old_feature, u32 new_feature > >> > { > >> > struct audit_buffer *ab; > >> > > >> > - if (audit_enabled == AUDIT_OFF) > >> > + if (!audit_enabled) > >> > >> Sooo, this is an unrelated style change, why? Looking at the rest of > >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > >> why are you adding noise to this patch? > > > > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 > > instances where it could be used as a boolean where the expression is > > made harder to read (in my opinion). I thought it was worth changing to > > read the same way most of the other instances I've been reviewing are > > written. There are only two where the non-boolean distiction with > > AUDIT_LOCKED is required. > > Thanks for the explanation. > > While I still believe this patch, and connecting records in general, > is the Right Thing To Do, I'm expecting there to be some hate mail on > this issue and I would like to keep the patch as small and as > straight-to-the-point as possible just so there is little confusion > about what is changing. > > Please respin this without the style change and I'll merge it as soon > as I see it. Alternatively, give me the "ok" and I'll merge the patch > now and just drop the style change; after all we're talking about one > line in a five line patch ;) Go ahead and drop that style change line to simplify this patch and I'll submit another patch to clean them all up at the same time (probably the next time one of those changes). > >> > return; > >> > - > >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > >> > AUDIT_FEATURE_CHANGE); > >> > >> This is the important part, and the Right Thing To Do. > >> > >> > if (!ab) > >> > return; > >> > audit_log_task_info(ab, current); > > -- > paul moore > www.paul-moore.com - RGB -- Richard Guy Briggs 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
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggswrote: > On 2018-04-17 18:06, Paul Moore wrote: >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> > user action. >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> > >> > Signed-off-by: Richard Guy Briggs >> > --- >> > kernel/audit.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/kernel/audit.c b/kernel/audit.c >> > index 8da24ef..23f125b 100644 >> > --- a/kernel/audit.c >> > +++ b/kernel/audit.c >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 >> > old_feature, u32 new_feature >> > { >> > struct audit_buffer *ab; >> > >> > - if (audit_enabled == AUDIT_OFF) >> > + if (!audit_enabled) >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> why are you adding noise to this patch? > > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 > instances where it could be used as a boolean where the expression is > made harder to read (in my opinion). I thought it was worth changing to > read the same way most of the other instances I've been reviewing are > written. There are only two where the non-boolean distiction with > AUDIT_LOCKED is required. Thanks for the explanation. While I still believe this patch, and connecting records in general, is the Right Thing To Do, I'm expecting there to be some hate mail on this issue and I would like to keep the patch as small and as straight-to-the-point as possible just so there is little confusion about what is changing. Please respin this without the style change and I'll merge it as soon as I see it. Alternatively, give me the "ok" and I'll merge the patch now and just drop the style change; after all we're talking about one line in a five line patch ;) >> > return; >> > - >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, >> > AUDIT_FEATURE_CHANGE); >> >> This is the important part, and the Right Thing To Do. >> >> > if (!ab) >> > return; >> > audit_log_task_info(ab, current); -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Fri, Apr 20, 2018 at 9:46 AM, Richard Guy Briggs wrote: > On 2018-04-17 18:06, Paul Moore wrote: >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> > user action. >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> > >> > Signed-off-by: Richard Guy Briggs >> > --- >> > kernel/audit.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/kernel/audit.c b/kernel/audit.c >> > index 8da24ef..23f125b 100644 >> > --- a/kernel/audit.c >> > +++ b/kernel/audit.c >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 >> > old_feature, u32 new_feature >> > { >> > struct audit_buffer *ab; >> > >> > - if (audit_enabled == AUDIT_OFF) >> > + if (!audit_enabled) >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> why are you adding noise to this patch? > > Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 > instances where it could be used as a boolean where the expression is > made harder to read (in my opinion). I thought it was worth changing to > read the same way most of the other instances I've been reviewing are > written. There are only two where the non-boolean distiction with > AUDIT_LOCKED is required. Thanks for the explanation. While I still believe this patch, and connecting records in general, is the Right Thing To Do, I'm expecting there to be some hate mail on this issue and I would like to keep the patch as small and as straight-to-the-point as possible just so there is little confusion about what is changing. Please respin this without the style change and I'll merge it as soon as I see it. Alternatively, give me the "ok" and I'll merge the patch now and just drop the style change; after all we're talking about one line in a five line patch ;) >> > return; >> > - >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, >> > AUDIT_FEATURE_CHANGE); >> >> This is the important part, and the Right Thing To Do. >> >> > if (!ab) >> > return; >> > audit_log_task_info(ab, current); -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On 2018-04-17 18:06, Paul Moore wrote: > On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggswrote: > > Tie syscall information to FEATURE_CHANGE calls since it is a result of > > user action. > > > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > > > Signed-off-by: Richard Guy Briggs > > --- > > kernel/audit.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 8da24ef..23f125b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 > > old_feature, u32 new_feature > > { > > struct audit_buffer *ab; > > > > - if (audit_enabled == AUDIT_OFF) > > + if (!audit_enabled) > > Sooo, this is an unrelated style change, why? Looking at the rest of > kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > why are you adding noise to this patch? Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 instances where it could be used as a boolean where the expression is made harder to read (in my opinion). I thought it was worth changing to read the same way most of the other instances I've been reviewing are written. There are only two where the non-boolean distiction with AUDIT_LOCKED is required. > > return; > > - > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > > AUDIT_FEATURE_CHANGE); > > This is the important part, and the Right Thing To Do. > > > if (!ab) > > return; > > audit_log_task_info(ab, current); > > paul moore - RGB -- Richard Guy Briggs 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
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On 2018-04-17 18:06, Paul Moore wrote: > On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: > > Tie syscall information to FEATURE_CHANGE calls since it is a result of > > user action. > > > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > > > Signed-off-by: Richard Guy Briggs > > --- > > kernel/audit.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 8da24ef..23f125b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 > > old_feature, u32 new_feature > > { > > struct audit_buffer *ab; > > > > - if (audit_enabled == AUDIT_OFF) > > + if (!audit_enabled) > > Sooo, this is an unrelated style change, why? Looking at the rest of > kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > why are you adding noise to this patch? Ok, survey sez 25 instances of audit_enabled used as a boolean vs 7 instances where it could be used as a boolean where the expression is made harder to read (in my opinion). I thought it was worth changing to read the same way most of the other instances I've been reviewing are written. There are only two where the non-boolean distiction with AUDIT_LOCKED is required. > > return; > > - > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > > AUDIT_FEATURE_CHANGE); > > This is the important part, and the Right Thing To Do. > > > if (!ab) > > return; > > audit_log_task_info(ab, current); > > paul moore - RGB -- Richard Guy Briggs 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
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Tue, Apr 17, 2018 at 6:10 PM, Steve Grubbwrote: > On Tuesday, April 17, 2018 6:06:24 PM EDT Paul Moore wrote: >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> > user action. >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> > >> > Signed-off-by: Richard Guy Briggs >> > --- >> > >> > kernel/audit.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/kernel/audit.c b/kernel/audit.c >> > index 8da24ef..23f125b 100644 >> > --- a/kernel/audit.c >> > +++ b/kernel/audit.c >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, >> > u32 old_feature, u32 new_feature> >> > { >> > >> > struct audit_buffer *ab; >> > >> > - if (audit_enabled == AUDIT_OFF) >> > + if (!audit_enabled) >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> why are you adding noise to this patch? >> >> > return; >> > >> > - >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, >> > AUDIT_FEATURE_CHANGE); >> This is the important part, and the Right Thing To Do. > > This is an unexpected change. I have asked questions on the github issue > tracker but have not gotten a satisfactory answer. Please do not merge this > until there's agreement on this. It shouldn't be surprising, we've been talking about connecting records for some time now, in different contexts and both on and off list. Not only does it helps pave the way for the audit container ID work, it just makes sense. I've seen your questions in this particular GitHub issue and I think Richard has answered them satisfactorily. Once Richard removes the style change, or provides a good enough reason for why it should stay in this patch, I plan on merging this into audit/next. -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Tue, Apr 17, 2018 at 6:10 PM, Steve Grubb wrote: > On Tuesday, April 17, 2018 6:06:24 PM EDT Paul Moore wrote: >> On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: >> > Tie syscall information to FEATURE_CHANGE calls since it is a result of >> > user action. >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/80 >> > >> > Signed-off-by: Richard Guy Briggs >> > --- >> > >> > kernel/audit.c | 5 ++--- >> > 1 file changed, 2 insertions(+), 3 deletions(-) >> > >> > diff --git a/kernel/audit.c b/kernel/audit.c >> > index 8da24ef..23f125b 100644 >> > --- a/kernel/audit.c >> > +++ b/kernel/audit.c >> > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, >> > u32 old_feature, u32 new_feature> >> > { >> > >> > struct audit_buffer *ab; >> > >> > - if (audit_enabled == AUDIT_OFF) >> > + if (!audit_enabled) >> >> Sooo, this is an unrelated style change, why? Looking at the rest of >> kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so >> why are you adding noise to this patch? >> >> > return; >> > >> > - >> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); >> > + ab = audit_log_start(current->audit_context, GFP_KERNEL, >> > AUDIT_FEATURE_CHANGE); >> This is the important part, and the Right Thing To Do. > > This is an unexpected change. I have asked questions on the github issue > tracker but have not gotten a satisfactory answer. Please do not merge this > until there's agreement on this. It shouldn't be surprising, we've been talking about connecting records for some time now, in different contexts and both on and off list. Not only does it helps pave the way for the audit container ID work, it just makes sense. I've seen your questions in this particular GitHub issue and I think Richard has answered them satisfactorily. Once Richard removes the style change, or provides a good enough reason for why it should stay in this patch, I plan on merging this into audit/next. -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Tuesday, April 17, 2018 6:06:24 PM EDT Paul Moore wrote: > On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggswrote: > > Tie syscall information to FEATURE_CHANGE calls since it is a result of > > user action. > > > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > > > Signed-off-by: Richard Guy Briggs > > --- > > > > kernel/audit.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 8da24ef..23f125b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, > > u32 old_feature, u32 new_feature> > > { > > > > struct audit_buffer *ab; > > > > - if (audit_enabled == AUDIT_OFF) > > + if (!audit_enabled) > > Sooo, this is an unrelated style change, why? Looking at the rest of > kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > why are you adding noise to this patch? > > > return; > > > > - > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > > AUDIT_FEATURE_CHANGE); > This is the important part, and the Right Thing To Do. This is an unexpected change. I have asked questions on the github issue tracker but have not gotten a satisfactory answer. Please do not merge this until there's agreement on this. Thanks, -Steve > > if (!ab) > > > > return; > > > > audit_log_task_info(ab, current); > > > > -- > > 1.8.3.1
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Tuesday, April 17, 2018 6:06:24 PM EDT Paul Moore wrote: > On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: > > Tie syscall information to FEATURE_CHANGE calls since it is a result of > > user action. > > > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > > > Signed-off-by: Richard Guy Briggs > > --- > > > > kernel/audit.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 8da24ef..23f125b 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, > > u32 old_feature, u32 new_feature> > > { > > > > struct audit_buffer *ab; > > > > - if (audit_enabled == AUDIT_OFF) > > + if (!audit_enabled) > > Sooo, this is an unrelated style change, why? Looking at the rest of > kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so > why are you adding noise to this patch? > > > return; > > > > - > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > > AUDIT_FEATURE_CHANGE); > This is the important part, and the Right Thing To Do. This is an unexpected change. I have asked questions on the github issue tracker but have not gotten a satisfactory answer. Please do not merge this until there's agreement on this. Thanks, -Steve > > if (!ab) > > > > return; > > > > audit_log_task_info(ab, current); > > > > -- > > 1.8.3.1
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggswrote: > Tie syscall information to FEATURE_CHANGE calls since it is a result of > user action. > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > Signed-off-by: Richard Guy Briggs > --- > kernel/audit.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 8da24ef..23f125b 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 > old_feature, u32 new_feature > { > struct audit_buffer *ab; > > - if (audit_enabled == AUDIT_OFF) > + if (!audit_enabled) Sooo, this is an unrelated style change, why? Looking at the rest of kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so why are you adding noise to this patch? > return; > - > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > AUDIT_FEATURE_CHANGE); This is the important part, and the Right Thing To Do. > if (!ab) > return; > audit_log_task_info(ab, current); > -- > 1.8.3.1 -- paul moore www.paul-moore.com
Re: [PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
On Wed, Apr 11, 2018 at 8:46 AM, Richard Guy Briggs wrote: > Tie syscall information to FEATURE_CHANGE calls since it is a result of > user action. > > See: https://github.com/linux-audit/audit-kernel/issues/80 > > Signed-off-by: Richard Guy Briggs > --- > kernel/audit.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 8da24ef..23f125b 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 > old_feature, u32 new_feature > { > struct audit_buffer *ab; > > - if (audit_enabled == AUDIT_OFF) > + if (!audit_enabled) Sooo, this is an unrelated style change, why? Looking at the rest of kernel/audit.c we seem to use a mix of "(!x)" and "(x == 0/CONST)" so why are you adding noise to this patch? > return; > - > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); > + ab = audit_log_start(current->audit_context, GFP_KERNEL, > AUDIT_FEATURE_CHANGE); This is the important part, and the Right Thing To Do. > if (!ab) > return; > audit_log_task_info(ab, current); > -- > 1.8.3.1 -- paul moore www.paul-moore.com
[PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
Tie syscall information to FEATURE_CHANGE calls since it is a result of user action. See: https://github.com/linux-audit/audit-kernel/issues/80 Signed-off-by: Richard Guy Briggs--- kernel/audit.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 8da24ef..23f125b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature { struct audit_buffer *ab; - if (audit_enabled == AUDIT_OFF) + if (!audit_enabled) return; - - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); + ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE); if (!ab) return; audit_log_task_info(ab, current); -- 1.8.3.1
[PATCH ghak80 V1] audit: add syscall information to FEATURE_CHANGE records
Tie syscall information to FEATURE_CHANGE calls since it is a result of user action. See: https://github.com/linux-audit/audit-kernel/issues/80 Signed-off-by: Richard Guy Briggs --- kernel/audit.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 8da24ef..23f125b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1103,10 +1103,9 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature { struct audit_buffer *ab; - if (audit_enabled == AUDIT_OFF) + if (!audit_enabled) return; - - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE); + ab = audit_log_start(current->audit_context, GFP_KERNEL, AUDIT_FEATURE_CHANGE); if (!ab) return; audit_log_task_info(ab, current); -- 1.8.3.1