| Message ID | 1431672946-300-3-git-send-email-ying.xue@windriver.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote: > Once modifying a pending timer of a neighbour, it's insufficient to > post a warning message. Instead we should not take the neighbour's > reference count at the same time, otherwise, it causes an issue that > the neighbour cannot be freed forever. > > Signed-off-by: Ying Xue <ying.xue@windriver.com> > --- > net/core/neighbour.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c > index 3de6542..5595db3 100644 > --- a/net/core/neighbour.c > +++ b/net/core/neighbour.c > @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl) > > static void neigh_add_timer(struct neighbour *n, unsigned long when) > { > - neigh_hold(n); > - if (unlikely(mod_timer(&n->timer, when))) { > - printk("NEIGH: BUG, double timer add, state is %x\n", > - n->nud_state); > + if (likely(!mod_timer(&n->timer, when))) { > + neigh_hold(n); > + } else { > + pr_warn("NEIGH: BUG, double timer add, state is %x\n", > + n->nud_state); > dump_stack(); > } > } NACK -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 15 May 2015 05:12:42 -0700 > On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote: >> Once modifying a pending timer of a neighbour, it's insufficient to >> post a warning message. Instead we should not take the neighbour's >> reference count at the same time, otherwise, it causes an issue that >> the neighbour cannot be freed forever. >> >> Signed-off-by: Ying Xue <ying.xue@windriver.com> >> --- >> net/core/neighbour.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/net/core/neighbour.c b/net/core/neighbour.c >> index 3de6542..5595db3 100644 >> --- a/net/core/neighbour.c >> +++ b/net/core/neighbour.c >> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl) >> >> static void neigh_add_timer(struct neighbour *n, unsigned long when) >> { >> - neigh_hold(n); >> - if (unlikely(mod_timer(&n->timer, when))) { >> - printk("NEIGH: BUG, double timer add, state is %x\n", >> - n->nud_state); >> + if (likely(!mod_timer(&n->timer, when))) { >> + neigh_hold(n); >> + } else { >> + pr_warn("NEIGH: BUG, double timer add, state is %x\n", >> + n->nud_state); >> dump_stack(); >> } >> } > > > NACK Indeed, major NACK. And you've been told this change is unacceptable multiple times already, and you've been told exactly why as well. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2015 11:39 PM, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Fri, 15 May 2015 05:12:42 -0700 > >> On Fri, 2015-05-15 at 14:55 +0800, Ying Xue wrote: >>> Once modifying a pending timer of a neighbour, it's insufficient to >>> post a warning message. Instead we should not take the neighbour's >>> reference count at the same time, otherwise, it causes an issue that >>> the neighbour cannot be freed forever. >>> >>> Signed-off-by: Ying Xue <ying.xue@windriver.com> >>> --- >>> net/core/neighbour.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c >>> index 3de6542..5595db3 100644 >>> --- a/net/core/neighbour.c >>> +++ b/net/core/neighbour.c >>> @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl) >>> >>> static void neigh_add_timer(struct neighbour *n, unsigned long when) >>> { >>> - neigh_hold(n); >>> - if (unlikely(mod_timer(&n->timer, when))) { >>> - printk("NEIGH: BUG, double timer add, state is %x\n", >>> - n->nud_state); >>> + if (likely(!mod_timer(&n->timer, when))) { >>> + neigh_hold(n); >>> + } else { >>> + pr_warn("NEIGH: BUG, double timer add, state is %x\n", >>> + n->nud_state); >>> dump_stack(); >>> } >>> } >> >> >> NACK > > Indeed, major NACK. And you've been told this change is unacceptable > multiple times already, and you've been told exactly why as well. > Eric, according to your below comments for the single patch, http://www.spinics.net/lists/netdev/msg328828.html I supposed the reason why you rejected it was because the issue of "neigh use-after-free" was not resolved yet. More importantly, you did not explicitly explain why the patch was wrong although I stated the problem the patch tries to fix is different with the one discussed in the thread: http://www.spinics.net/lists/netdev/msg323883.html So, regarding my understanding for your comment, the patch can be acceptable if the issue of "neigh use-after-free" is solved. Since then I started to investigate its root cause and created the first patch attempting to fix it. This is why I involve the patch into the series again. If the issue of "neigh use-after-free" is fixed by the first patch although you said the atomic_read() was not safe for us, is the patch still wrong? If it's really wrong, can you please give more detailed explanation to help me understand why the change is wrong and but why a similar timer usage in sk_reset_timer() is not wrong? Thanks, Ying -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-05-18 at 11:24 +0800, Ying Xue wrote: > If the issue of "neigh use-after-free" is fixed by the first patch although you > said the atomic_read() was not safe for us, is the patch still wrong? If it's > really wrong, can you please give more detailed explanation to help me > understand why the change is wrong and but why a similar timer usage in > sk_reset_timer() is not wrong? Why do you believe sk_reset_timer() would be wrong ? Difference between neigh_add_timer() and sk_reset_timer() is very simple : neigh_add_timer() must be called while the timer is not yet armed. sk_reset_timer() can be called while timer is already armed. You are changing neigh_add_timer() for no good reason, just because you want it to be 'like sk_reset_timer()' ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/18/2015 12:58 PM, Eric Dumazet wrote: > On Mon, 2015-05-18 at 11:24 +0800, Ying Xue wrote: > >> If the issue of "neigh use-after-free" is fixed by the first patch although you >> said the atomic_read() was not safe for us, is the patch still wrong? If it's >> really wrong, can you please give more detailed explanation to help me >> understand why the change is wrong and but why a similar timer usage in >> sk_reset_timer() is not wrong? > > Why do you believe sk_reset_timer() would be wrong ? > Sorry, I don't think sk_reset_timer() is wrong, instead I suppose neigh_add_timer() is wrong :) > Difference between neigh_add_timer() and sk_reset_timer() is very > simple : > > neigh_add_timer() must be called while the timer is not yet armed. > In case that the caller of neigh_add_timer() attempts to modify an active timer due to a bug or something wrong else, why not prevent neigh_add_timer() from taking neigh refcnt beside posting a warning message? So, exactly speaking, we cannot say neigh_add_timer() is completely wrong regarding your mentioned above assumption that neigh timer is absolutely not armed when neigh_add_timer() is called. But we can say its behaviour is not designed very well. From this point, the patch seems still a bit valuable for us. Regards, Ying > sk_reset_timer() can be called while timer is already armed. > > You are changing neigh_add_timer() for no good reason, just because you > want it to be 'like sk_reset_timer()' ? > > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2015-05-18 at 13:55 +0800, Ying Xue wrote: > So, exactly speaking, we cannot say neigh_add_timer() is completely wrong > regarding your mentioned above assumption that neigh timer is absolutely not > armed when neigh_add_timer() is called. But we can say its behaviour is not > designed very well. From this point, the patch seems still a bit valuable for us. Its design is correct. We have to fix the caller that does not respect the proper convention. So please try to understand the current logic, instead of changing everything, and adding another bunch of bugs. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/neighbour.c b/net/core/neighbour.c index 3de6542..5595db3 100644 --- a/net/core/neighbour.c +++ b/net/core/neighbour.c @@ -164,10 +164,11 @@ static int neigh_forced_gc(struct neigh_table *tbl) static void neigh_add_timer(struct neighbour *n, unsigned long when) { - neigh_hold(n); - if (unlikely(mod_timer(&n->timer, when))) { - printk("NEIGH: BUG, double timer add, state is %x\n", - n->nud_state); + if (likely(!mod_timer(&n->timer, when))) { + neigh_hold(n); + } else { + pr_warn("NEIGH: BUG, double timer add, state is %x\n", + n->nud_state); dump_stack(); } }
Once modifying a pending timer of a neighbour, it's insufficient to post a warning message. Instead we should not take the neighbour's reference count at the same time, otherwise, it causes an issue that the neighbour cannot be freed forever. Signed-off-by: Ying Xue <ying.xue@windriver.com> --- net/core/neighbour.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)