| Message ID | 1487585948-6401-16-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova <elena.reshetova@intel.com> wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > Signed-off-by: David Windsor <dwindsor@gmail.com> > --- > kernel/audit_tree.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) No objection on my end, same for patch 16/19. I have no problem merging both these patches into the audit/next branch after the merge window, is that your goal or are you merging these via a different tree? > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > index 7b44195..7ed617b 100644 > --- a/kernel/audit_tree.c > +++ b/kernel/audit_tree.c > @@ -9,7 +9,7 @@ struct audit_tree; > struct audit_chunk; > > struct audit_tree { > - atomic_t count; > + refcount_t count; > int goner; > struct audit_chunk *root; > struct list_head chunks; > @@ -77,7 +77,7 @@ static struct audit_tree *alloc_tree(const char *s) > > tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); > if (tree) { > - atomic_set(&tree->count, 1); > + refcount_set(&tree->count, 1); > tree->goner = 0; > INIT_LIST_HEAD(&tree->chunks); > INIT_LIST_HEAD(&tree->rules); > @@ -91,12 +91,12 @@ static struct audit_tree *alloc_tree(const char *s) > > static inline void get_tree(struct audit_tree *tree) > { > - atomic_inc(&tree->count); > + refcount_inc(&tree->count); > } > > static inline void put_tree(struct audit_tree *tree) > { > - if (atomic_dec_and_test(&tree->count)) > + if (refcount_dec_and_test(&tree->count)) > kfree_rcu(tree, head); > } >
> On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova > <elena.reshetova@intel.com> wrote: > > refcount_t type and corresponding API should be > > used instead of atomic_t when the variable is used as > > a reference counter. This allows to avoid accidental > > refcounter overflows that might lead to use-after-free > > situations. > > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Signed-off-by: David Windsor <dwindsor@gmail.com> > > --- > > kernel/audit_tree.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > No objection on my end, same for patch 16/19. > > I have no problem merging both these patches into the audit/next > branch after the merge window, is that your goal or are you merging > these via a different tree? Thank you Paul! I think it is better if they go through the trees they supposed to go through since this way they would get more testing and etc. So, please take the relevant ones to your tree when the time is right. After the first round, I guess we will see what patches are not propagating and then maybe take them via Kees tree. Best Regards, Elena. > > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index 7b44195..7ed617b 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -9,7 +9,7 @@ struct audit_tree; > > struct audit_chunk; > > > > struct audit_tree { > > - atomic_t count; > > + refcount_t count; > > int goner; > > struct audit_chunk *root; > > struct list_head chunks; > > @@ -77,7 +77,7 @@ static struct audit_tree *alloc_tree(const char *s) > > > > tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); > > if (tree) { > > - atomic_set(&tree->count, 1); > > + refcount_set(&tree->count, 1); > > tree->goner = 0; > > INIT_LIST_HEAD(&tree->chunks); > > INIT_LIST_HEAD(&tree->rules); > > @@ -91,12 +91,12 @@ static struct audit_tree *alloc_tree(const char *s) > > > > static inline void get_tree(struct audit_tree *tree) > > { > > - atomic_inc(&tree->count); > > + refcount_inc(&tree->count); > > } > > > > static inline void put_tree(struct audit_tree *tree) > > { > > - if (atomic_dec_and_test(&tree->count)) > > + if (refcount_dec_and_test(&tree->count)) > > kfree_rcu(tree, head); > > } > > > > -- > paul moore > www.paul-moore.com
On Tue, Feb 21, 2017 at 2:15 AM, Reshetova, Elena <elena.reshetova@intel.com> wrote: >> On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova >> <elena.reshetova@intel.com> wrote: >> > refcount_t type and corresponding API should be >> > used instead of atomic_t when the variable is used as >> > a reference counter. This allows to avoid accidental >> > refcounter overflows that might lead to use-after-free >> > situations. >> > >> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> >> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> >> > Signed-off-by: Kees Cook <keescook@chromium.org> >> > Signed-off-by: David Windsor <dwindsor@gmail.com> >> > --- >> > kernel/audit_tree.c | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> >> No objection on my end, same for patch 16/19. >> >> I have no problem merging both these patches into the audit/next >> branch after the merge window, is that your goal or are you merging >> these via a different tree? > > Thank you Paul! I think it is better if they go through the trees they supposed to go through > since this way they would get more testing and etc. So, please take the relevant ones to your tree when the time is right. > > After the first round, I guess we will see what patches are not propagating and then maybe take them via Kees tree. I just realized that include/linux/refcount.h didn't make it into v4.10 which means there is going to be delay until I merge them into the audit tree (I don't base the tree on -rc releases except under extreme circumstances). I've got the patches queued up in a private holding branch (I added #includes BTW) so I won't forget, but as a FYI, they likely won't make it in until v4.12.
On Tue, Feb 28, 2017 at 2:11 PM, Paul Moore <paul@paul-moore.com> wrote: > On Tue, Feb 21, 2017 at 2:15 AM, Reshetova, Elena > <elena.reshetova@intel.com> wrote: >>> On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova >>> <elena.reshetova@intel.com> wrote: >>> > refcount_t type and corresponding API should be >>> > used instead of atomic_t when the variable is used as >>> > a reference counter. This allows to avoid accidental >>> > refcounter overflows that might lead to use-after-free >>> > situations. >>> > >>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> >>> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> >>> > Signed-off-by: Kees Cook <keescook@chromium.org> >>> > Signed-off-by: David Windsor <dwindsor@gmail.com> >>> > --- >>> > kernel/audit_tree.c | 8 ++++---- >>> > 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> No objection on my end, same for patch 16/19. >>> >>> I have no problem merging both these patches into the audit/next >>> branch after the merge window, is that your goal or are you merging >>> these via a different tree? >> >> Thank you Paul! I think it is better if they go through the trees they supposed to go through >> since this way they would get more testing and etc. So, please take the relevant ones to your tree when the time is right. >> >> After the first round, I guess we will see what patches are not propagating and then maybe take them via Kees tree. > > I just realized that include/linux/refcount.h didn't make it into > v4.10 which means there is going to be delay until I merge them into > the audit tree (I don't base the tree on -rc releases except under > extreme circumstances). I've got the patches queued up in a private > holding branch (I added #includes BTW) so I won't forget, but as a > FYI, they likely won't make it in until v4.12. I'm not asking for you to change this, but I am curious: doesn't that force you to always be a release behind? I've tended to base trees on -rc2 (and then the final release while the next merge window is open). But that may be because I tend to have such wide dependencies... -Kees
On Tue, Feb 28, 2017 at 7:16 PM, Kees Cook <keescook@chromium.org> wrote: > On Tue, Feb 28, 2017 at 2:11 PM, Paul Moore <paul@paul-moore.com> wrote: >> I just realized that include/linux/refcount.h didn't make it into >> v4.10 which means there is going to be delay until I merge them into >> the audit tree (I don't base the tree on -rc releases except under >> extreme circumstances). I've got the patches queued up in a private >> holding branch (I added #includes BTW) so I won't forget, but as a >> FYI, they likely won't make it in until v4.12. > > I'm not asking for you to change this, but I am curious: doesn't that > force you to always be a release behind? I've tended to base trees on > -rc2 (and then the final release while the next merge window is open). > But that may be because I tend to have such wide dependencies... In general, yes ... and if you are just looking for the short answer I'd leave it at that. I do leave door open for exceptions under unusual circumstances, but I don't believe the refcount_t conversion is one of those cases. The longer answer lies in a balancing act between what I understand Linus' and/or James desires (different upstream maintainers, different approaches, but for my own sanity I like to stick to a single "process" across my trees), the linux-next effort, branch stability (aka limited or predictable rebases), and my own testing requirements. First off, the current policy I follow for the SELinux and audit trees can be found here: * http://www.paul-moore.com/blog/d/2016/03/kernel-repo-process.html ... it's relatively simple and has proven to work reasonably well over the past year or so. On occasion, the subsystem changes in a given release are significant enough that I skip a rebase (step #2 in the process) but that has only happened once (twice?) with the audit tree and didn't prove to be a real problem; this is less of an issue with the SELinux tree as James often rebases the linux-security tree to the current -rc1 or -rc2 tree. As for the balancing act ... My understanding is that Linus doesn't like to see pull requests from trees based on -rcX tags, he would much prefer to see trees based on a proper release, e.g. v4.10; on the plus side, Linus is willing to put up with some merge fuzzing so long as it is understandable and/or well explained. James wants pull requests that apply with zero merge conflicts to his linux-security/next tree; on the plus side, the linux-security/next tree tends to be based on the current -rc1/2 so a broad set of dependencies isn't too bad (which is important for SELinux). The linux-next people want to see a commit ID follow a steady progression of multiple weeks in the subsystem -next branch and then a trickle up through various trees until it hits Linus' tree. The branch stability requirements are pretty obvious, and with the exception of the -next branches during/immediately-after the merge window, I don't really rebase branches unless there is a Very Good Reason. Finally, my own testing requirements are such that I want to test the current SELinux and audit -next/-stable branches against the latest bits in Linus' tree (e.g. -rcX releases) on a weekly basis so keeping those branches as current as possible is important; I talked a bit more about my testing at Flock 2016, slides/video at the link below: * http://www.paul-moore.com/blog/d/2016/08/flock-kernel-testing.html * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-secnext There ya go, probably more than you wanted to know, but that's why things are the way they are with the SELinux and audit trees. I remain open to new ideas about how to manage the trees, but the current arrangement seems to work reasonably well at the moment.
On Wed, Mar 1, 2017 at 11:35 AM, Paul Moore <paul@paul-moore.com> wrote: > On Tue, Feb 28, 2017 at 7:16 PM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Feb 28, 2017 at 2:11 PM, Paul Moore <paul@paul-moore.com> wrote: >>> I just realized that include/linux/refcount.h didn't make it into >>> v4.10 which means there is going to be delay until I merge them into >>> the audit tree (I don't base the tree on -rc releases except under >>> extreme circumstances). I've got the patches queued up in a private >>> holding branch (I added #includes BTW) so I won't forget, but as a >>> FYI, they likely won't make it in until v4.12. >> >> I'm not asking for you to change this, but I am curious: doesn't that >> force you to always be a release behind? I've tended to base trees on >> -rc2 (and then the final release while the next merge window is open). >> But that may be because I tend to have such wide dependencies... > > In general, yes ... and if you are just looking for the short answer > I'd leave it at that. I do leave door open for exceptions under > unusual circumstances, but I don't believe the refcount_t conversion > is one of those cases. Right, totally agreed: doing refcount_t conversions is going to be a long tail process, which is fine. > The longer answer lies in a balancing act between what I understand > Linus' and/or James desires (different upstream maintainers, different > approaches, but for my own sanity I like to stick to a single > "process" across my trees), the linux-next effort, branch stability > (aka limited or predictable rebases), and my own testing requirements. > First off, the current policy I follow for the SELinux and audit trees > can be found here: > > * http://www.paul-moore.com/blog/d/2016/03/kernel-repo-process.html > > ... it's relatively simple and has proven to work reasonably well over > the past year or so. On occasion, the subsystem changes in a given > release are significant enough that I skip a rebase (step #2 in the > process) but that has only happened once (twice?) with the audit tree > and didn't prove to be a real problem; this is less of an issue with > the SELinux tree as James often rebases the linux-security tree to the > current -rc1 or -rc2 tree. > > As for the balancing act ... My understanding is that Linus doesn't > like to see pull requests from trees based on -rcX tags, he would much > prefer to see trees based on a proper release, e.g. v4.10; on the plus > side, Linus is willing to put up with some merge fuzzing so long as it > is understandable and/or well explained. Ah, hrm, yeah, I'd heard Linus mention that in the past, but when I was talking with sfr about what's best for -next tree targets, he mentioned -rc2 since it tends (in my understanding/memory of what he told me) to be the least "noisy" position. Interesting! > James wants pull requests > that apply with zero merge conflicts to his linux-security/next tree; > on the plus side, the linux-security/next tree tends to be based on > the current -rc1/2 so a broad set of dependencies isn't too bad (which > is important for SELinux). The linux-next people want to see a commit > ID follow a steady progression of multiple weeks in the subsystem > -next branch and then a trickle up through various trees until it hits > Linus' tree. The branch stability requirements are pretty obvious, > and with the exception of the -next branches during/immediately-after > the merge window, I don't really rebase branches unless there is a > Very Good Reason. Finally, my own testing requirements are such that > I want to test the current SELinux and audit -next/-stable branches > against the latest bits in Linus' tree (e.g. -rcX releases) on a > weekly basis so keeping those branches as current as possible is > important; I talked a bit more about my testing at Flock 2016, > slides/video at the link below: > > * http://www.paul-moore.com/blog/d/2016/08/flock-kernel-testing.html > * https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-secnext > > There ya go, probably more than you wanted to know, but that's why > things are the way they are with the SELinux and audit trees. I > remain open to new ideas about how to manage the trees, but the > current arrangement seems to work reasonably well at the moment. Actually, no, this was great. Thanks! It's why I asked. :) Whenever I see a maintainer with a different approach I try to figure out what I'm missing, etc. :) -Kees
On Tue, Feb 28, 2017 at 5:11 PM, Paul Moore <paul@paul-moore.com> wrote: > On Tue, Feb 21, 2017 at 2:15 AM, Reshetova, Elena > <elena.reshetova@intel.com> wrote: >>> On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova >>> <elena.reshetova@intel.com> wrote: >>> > refcount_t type and corresponding API should be >>> > used instead of atomic_t when the variable is used as >>> > a reference counter. This allows to avoid accidental >>> > refcounter overflows that might lead to use-after-free >>> > situations. >>> > >>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> >>> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> >>> > Signed-off-by: Kees Cook <keescook@chromium.org> >>> > Signed-off-by: David Windsor <dwindsor@gmail.com> >>> > --- >>> > kernel/audit_tree.c | 8 ++++---- >>> > 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> No objection on my end, same for patch 16/19. >>> >>> I have no problem merging both these patches into the audit/next >>> branch after the merge window, is that your goal or are you merging >>> these via a different tree? >> >> Thank you Paul! I think it is better if they go through the trees they supposed to go through >> since this way they would get more testing and etc. So, please take the relevant ones to your tree when the time is right. >> >> After the first round, I guess we will see what patches are not propagating and then maybe take them via Kees tree. > > I just realized that include/linux/refcount.h didn't make it into > v4.10 which means there is going to be delay until I merge them into > the audit tree (I don't base the tree on -rc releases except under > extreme circumstances). I've got the patches queued up in a private > holding branch (I added #includes BTW) so I won't forget, but as a > FYI, they likely won't make it in until v4.12. Quick update on this: I needed to rebase the audit/next branch for other reasons so I've gone ahead and merged 15/19 and 16/19 into audit/next; they should go to Linus during the next merge window. Thanks for your patience.
> On Tue, Feb 28, 2017 at 5:11 PM, Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Feb 21, 2017 at 2:15 AM, Reshetova, Elena > > <elena.reshetova@intel.com> wrote: > >>> On Mon, Feb 20, 2017 at 5:19 AM, Elena Reshetova > >>> <elena.reshetova@intel.com> wrote: > >>> > refcount_t type and corresponding API should be > >>> > used instead of atomic_t when the variable is used as > >>> > a reference counter. This allows to avoid accidental > >>> > refcounter overflows that might lead to use-after-free > >>> > situations. > >>> > > >>> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com> > >>> > Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com> > >>> > Signed-off-by: Kees Cook <keescook@chromium.org> > >>> > Signed-off-by: David Windsor <dwindsor@gmail.com> > >>> > --- > >>> > kernel/audit_tree.c | 8 ++++---- > >>> > 1 file changed, 4 insertions(+), 4 deletions(-) > >>> > >>> No objection on my end, same for patch 16/19. > >>> > >>> I have no problem merging both these patches into the audit/next > >>> branch after the merge window, is that your goal or are you merging > >>> these via a different tree? > >> > >> Thank you Paul! I think it is better if they go through the trees they supposed to > go through > >> since this way they would get more testing and etc. So, please take the relevant > ones to your tree when the time is right. > >> > >> After the first round, I guess we will see what patches are not propagating and > then maybe take them via Kees tree. > > > > I just realized that include/linux/refcount.h didn't make it into > > v4.10 which means there is going to be delay until I merge them into > > the audit tree (I don't base the tree on -rc releases except under > > extreme circumstances). I've got the patches queued up in a private > > holding branch (I added #includes BTW) so I won't forget, but as a > > FYI, they likely won't make it in until v4.12. > > Quick update on this: I needed to rebase the audit/next branch for > other reasons so I've gone ahead and merged 15/19 and 16/19 into > audit/next; they should go to Linus during the next merge window. > Thanks for your patience. Thank you very much Paul! I just got back from holidays, so catching up with the mail slowly. Best Regards, Elena. > > -- > paul moore > www.paul-moore.com
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 7b44195..7ed617b 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -9,7 +9,7 @@ struct audit_tree; struct audit_chunk; struct audit_tree { - atomic_t count; + refcount_t count; int goner; struct audit_chunk *root; struct list_head chunks; @@ -77,7 +77,7 @@ static struct audit_tree *alloc_tree(const char *s) tree = kmalloc(sizeof(struct audit_tree) + strlen(s) + 1, GFP_KERNEL); if (tree) { - atomic_set(&tree->count, 1); + refcount_set(&tree->count, 1); tree->goner = 0; INIT_LIST_HEAD(&tree->chunks); INIT_LIST_HEAD(&tree->rules); @@ -91,12 +91,12 @@ static struct audit_tree *alloc_tree(const char *s) static inline void get_tree(struct audit_tree *tree) { - atomic_inc(&tree->count); + refcount_inc(&tree->count); } static inline void put_tree(struct audit_tree *tree) { - if (atomic_dec_and_test(&tree->count)) + if (refcount_dec_and_test(&tree->count)) kfree_rcu(tree, head); }