| Message ID | 1500288206-12074-15-git-send-email-elena.reshetova@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 17 Jul 2017, Elena Reshetova wrote: > Subject: kernel: convert futex_pi_state.refcount from atomic_t to refcount_t Several people including myself told you already, that subjects consist of SUBSYSTEMPREFIX: Concise description It's easy enough to figure the prefix out by looking at the output of 'git log path/of/changed/file'. Concise descriptions are not lengthy sentences with implementation details. They should merily describe the problem/concept of change. The details go into the changelog. IOW, something like: "PROPERPREFIX: Convert to refcount API" would be sufficient. > 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. Copying the same sentence over and over avoids thinking about a proper changelog, right? You fail to explain, how you come to the conclusion that futex_pi_state.refcount is a pure reference counter (aside of the name) and therefor can be safely converted to refcount_t. Other than that, the patch itself is fine. Thanks, tglx
> On Mon, 17 Jul 2017, Elena Reshetova wrote: > > > Subject: kernel: convert futex_pi_state.refcount from atomic_t to refcount_t > > Several people including myself told you already, that subjects consist of > > SUBSYSTEMPREFIX: Concise description > > It's easy enough to figure the prefix out by looking at the output of 'git > log path/of/changed/file'. Ok, I will try this from now on. I didn't think of it, but was trying to figure it based on general location and meaning (obviously wrong). > > Concise descriptions are not lengthy sentences with implementation > details. They should merily describe the problem/concept of change. The > details go into the changelog. IOW, something like: > > "PROPERPREFIX: Convert to refcount API" > > would be sufficient. OK, will fix. > > > 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. > > Copying the same sentence over and over avoids thinking about a proper > changelog, right? You fail to explain, how you come to the conclusion that > futex_pi_state.refcount is a pure reference counter (aside of the name) and > therefor can be safely converted to refcount_t. OK, this is not very useful for many cases. Yes, I am using automated log on these patches, because I used to have 240 of them and writing manual logs for them would be fun. Moreover, in many cases, writing manual logs don't bring any value since I would have to repeat the same things all over again: xyz conversions was found by using *.cocci pattern first, then looked at manually and it looked like a standard reference counter that frees the things after calling refcount_dec_and_test() (or its variation with lock which is rare). Other things also looked correct, like I didn't see increments from zero, counter starts at 1 etc. I would really have to repeat the same thing in each changelog. Does it really bring value? Best Regards, Elena. > > Other than that, the patch itself is fine. > > Thanks, > > tglx
On Mon, 17 Jul 2017, Reshetova, Elena wrote: > > On Mon, 17 Jul 2017, Elena Reshetova 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. > > > > Copying the same sentence over and over avoids thinking about a proper > > changelog, right? You fail to explain, how you come to the conclusion that > > futex_pi_state.refcount is a pure reference counter (aside of the name) and > > therefor can be safely converted to refcount_t. > OK, this is not very useful for many cases. Yes, I am using automated log > on these patches, because I used to have 240 of them and writing manual > logs for them would be fun. Been there, done that. > Moreover, in many cases, writing manual logs don't bring any value since > I would have to repeat the same things all over again: xyz conversions > was found by using *.cocci pattern first, then looked at manually and it > looked like a standard reference counter that frees the things after > calling refcount_dec_and_test() (or its variation with lock which is > rare). Other things also looked correct, like I didn't see increments > from zero, counter starts at 1 etc. I would really have to repeat the > same thing in each changelog. Does it really bring value? You don't have to go into that level of detail, but you can provide enough information with a template as well, e.g.: atomic_t variables are often used to implement pure reference counters: - starting at 1 - freeing a resource after reaching zero - only using basic atomic operations (init, inc, dec_and_test) These variables should be converted to refcount_t because the refcount_t operations can catch and prevent accidental underflows and overflows. The variable FOO is used as a pure reference counter. Convert it to refcount_t and fix up the operations. That gives enough context for someone who looks at a patch because then the reviewer can look for: starts at 1, frees at 0, does not use any fancy operations and has not to use Gurgle to figure out what your understanding of reference counters is. Replacing FOO with the real variable name can be done with a script easy enough. Thanks, tglx
> On Mon, 17 Jul 2017, Reshetova, Elena wrote: > > > On Mon, 17 Jul 2017, Elena Reshetova 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. > > > > > > Copying the same sentence over and over avoids thinking about a proper > > > changelog, right? You fail to explain, how you come to the conclusion that > > > futex_pi_state.refcount is a pure reference counter (aside of the name) and > > > therefor can be safely converted to refcount_t. > > > OK, this is not very useful for many cases. Yes, I am using automated log > > on these patches, because I used to have 240 of them and writing manual > > logs for them would be fun. > > Been there, done that. > > > Moreover, in many cases, writing manual logs don't bring any value since > > I would have to repeat the same things all over again: xyz conversions > > was found by using *.cocci pattern first, then looked at manually and it > > looked like a standard reference counter that frees the things after > > calling refcount_dec_and_test() (or its variation with lock which is > > rare). Other things also looked correct, like I didn't see increments > > from zero, counter starts at 1 etc. I would really have to repeat the > > same thing in each changelog. Does it really bring value? > > You don't have to go into that level of detail, but you can provide enough > information with a template as well, e.g.: > > atomic_t variables are often used to implement pure reference counters: > - starting at 1 > - freeing a resource after reaching zero > - only using basic atomic operations (init, inc, dec_and_test) > > These variables should be converted to refcount_t because the refcount_t > operations can catch and prevent accidental underflows and overflows. > > The variable FOO is used as a pure reference counter. Convert it to > refcount_t and fix up the operations. > > That gives enough context for someone who looks at a patch because then the > reviewer can look for: > > starts at 1, frees at 0, does not use any fancy operations > > and has not to use Gurgle to figure out what your understanding of > reference counters is. > > Replacing FOO with the real variable name can be done with a script easy > enough. Ok, let me try updating the commits messages in the above way. As soon as I don't have to write them manually, I am fine with anything :) Best Regards, Elena. > > Thanks, > > tglx
diff --git a/kernel/futex.c b/kernel/futex.c index 16dbe4c..1cc7641 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -67,6 +67,7 @@ #include <linux/freezer.h> #include <linux/bootmem.h> #include <linux/fault-inject.h> +#include <linux/refcount.h> #include <asm/futex.h> @@ -209,7 +210,7 @@ struct futex_pi_state { struct rt_mutex pi_mutex; struct task_struct *owner; - atomic_t refcount; + refcount_t refcount; union futex_key key; } __randomize_layout; @@ -794,7 +795,7 @@ static int refill_pi_state_cache(void) INIT_LIST_HEAD(&pi_state->list); /* pi_mutex gets initialized later */ pi_state->owner = NULL; - atomic_set(&pi_state->refcount, 1); + refcount_set(&pi_state->refcount, 1); pi_state->key = FUTEX_KEY_INIT; current->pi_state_cache = pi_state; @@ -814,7 +815,7 @@ static struct futex_pi_state *alloc_pi_state(void) static void get_pi_state(struct futex_pi_state *pi_state) { - WARN_ON_ONCE(!atomic_inc_not_zero(&pi_state->refcount)); + refcount_inc(&pi_state->refcount); } /* @@ -828,7 +829,7 @@ static void put_pi_state(struct futex_pi_state *pi_state) if (!pi_state) return; - if (!atomic_dec_and_test(&pi_state->refcount)) + if (!refcount_dec_and_test(&pi_state->refcount)) return; /* @@ -852,7 +853,7 @@ static void put_pi_state(struct futex_pi_state *pi_state) * refcount is at 0 - put it back to 1. */ pi_state->owner = NULL; - atomic_set(&pi_state->refcount, 1); + refcount_set(&pi_state->refcount, 1); current->pi_state_cache = pi_state; } } @@ -1046,7 +1047,7 @@ static int attach_to_pi_state(u32 __user *uaddr, u32 uval, * and futex_wait_requeue_pi() as it cannot go to 0 and consequently * free pi_state before we can take a reference ourselves. */ - WARN_ON(!atomic_read(&pi_state->refcount)); + WARN_ON(!refcount_read(&pi_state->refcount)); /* * Now that we have a pi_state, we can acquire wait_lock