diff mbox

[2/3] ipvs: add missing lock in ip_vs_ftp_init_conn()

Message ID 1341965963-7275-3-git-send-email-horms@verge.net.au
State Not Applicable
Headers show

Commit Message

Simon Horman July 11, 2012, 12:19 a.m. UTC 
From: Xiaotian Feng <xtfeng@gmail.com>

We met a kernel panic in 2.6.32.43 kernel:

[2680191.848044] IPVS: ip_vs_conn_hash(): request for already hashed, called from run_timer_softirq+0x175/0x1d0
<snip>
[2680311.849009] general protection fault: 0000 [#1] SMP
[2680311.853001] RIP: 0010:[<ffffffff815f155c>]  [<ffffffff815f155c>] ip_vs_conn_expire+0xdc/0x2f0
[2680311.853001] RSP: 0018:ffff880028303e70  EFLAGS: 00010202
[2680311.853001] RAX: dead000000200200 RBX: ffff8801aad00b80 RCX: 0000000000001d90
[2680311.853001] RDX: dead000000100100 RSI: 000000004fd59800 RDI: ffff8801aad00c08
<snip>
[2680311.853001] Call Trace:
[2680311.853001]  <IRQ>
[2680311.853001]  [<ffffffff815f1480>] ? ip_vs_conn_expire+0x0/0x2f0
[2680311.853001]  [<ffffffff8104e2a5>] run_timer_softirq+0x175/0x1d0
[2680311.853001]  [<ffffffff81021a48>] ? lapic_next_event+0x18/0x20
[2680311.853001]  [<ffffffff81049a13>] __do_softirq+0xb3/0x150
[2680311.853001]  [<ffffffff8100cc5c>] call_softirq+0x1c/0x30
[2680311.853001]  [<ffffffff8100ea9a>] do_softirq+0x4a/0x80
[2680311.853001]  [<ffffffff81049957>] irq_exit+0x77/0x80
[2680311.853001]  [<ffffffff81021f2c>] smp_apic_timer_interrupt+0x6c/0xa0
[2680311.853001]  [<ffffffff8100c633>] apic_timer_interrupt+0x13/0x20
[2680311.853001]  <EOI>
[2680311.853001]  [<ffffffff81013b52>] ? mwait_idle+0x52/0x70
[2680311.853001]  [<ffffffff8100a7b0>] ? enter_idle+0x20/0x30
[2680311.853001]  [<ffffffff8100ac62>] ? cpu_idle+0x52/0x80
[2680311.853001]  [<ffffffff816d504d>] ? start_secondary+0x19d/0x280

rax and rdx is LIST_POISON1 and LIST_POISON2, so kernel is list_del() on an already deleted
connection and result the general protect fault.

The "request for already hashed" warning, told us someone might change the connection flags
incorrectly, like described in commit aea9d711, it changes the connection flags, but doesn't
put the connection back to the list. So ip_vs_conn_hash() throw a warning and return.
Later, when ip_vs_conn_expire fire again, ip_vs_conn_unhash() will find the HASHED connection
and list_del() it, then kernel panic happened.

After code review, the only chance that kernel change connection flag without protection is
in ip_vs_ftp_init_conn().

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: "David S. Miller" <davem@davemloft.net>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 net/netfilter/ipvs/ip_vs_ftp.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Pablo Neira Ayuso July 16, 2012, 9:07 p.m. UTC | #1 
Hi Simon,

On Wed, Jul 11, 2012 at 09:19:22AM +0900, Simon Horman wrote:
> From: Xiaotian Feng <xtfeng@gmail.com>
> 
> We met a kernel panic in 2.6.32.43 kernel:
[...]
>  net/netfilter/ipvs/ip_vs_ftp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> index b20b29c..c2bc264 100644
> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
> @@ -65,8 +65,10 @@ static int ip_vs_ftp_pasv;
>  static int
>  ip_vs_ftp_init_conn(struct ip_vs_app *app, struct ip_vs_conn *cp)
>  {
> +	spin_lock(&cp->lock);
>  	/* We use connection tracking for the command connection */
>  	cp->flags |= IP_VS_CONN_F_NFCT;
> +	spin_unlock(&cp->lock);
>  	return 0;

The conntrack support for FTP IPVS helper seems to be there since
2.6.37.

However, the patch description mentions 2.6.32.43.

Something doesn't match here, could you clarify this?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiaotian Feng July 17, 2012, 2:34 a.m. UTC | #2 
On Tue, Jul 17, 2012 at 5:07 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Simon,
>
> On Wed, Jul 11, 2012 at 09:19:22AM +0900, Simon Horman wrote:
>> From: Xiaotian Feng <xtfeng@gmail.com>
>>
>> We met a kernel panic in 2.6.32.43 kernel:
> [...]
>>  net/netfilter/ipvs/ip_vs_ftp.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
>> index b20b29c..c2bc264 100644
>> --- a/net/netfilter/ipvs/ip_vs_ftp.c
>> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
>> @@ -65,8 +65,10 @@ static int ip_vs_ftp_pasv;
>>  static int
>>  ip_vs_ftp_init_conn(struct ip_vs_app *app, struct ip_vs_conn *cp)
>>  {
>> +     spin_lock(&cp->lock);
>>       /* We use connection tracking for the command connection */
>>       cp->flags |= IP_VS_CONN_F_NFCT;
>> +     spin_unlock(&cp->lock);
>>       return 0;
>
> The conntrack support for FTP IPVS helper seems to be there since
> 2.6.37.
>
> However, the patch description mentions 2.6.32.43.
>
> Something doesn't match here, could you clarify this?
>

Sorry for the misleading description in the patch. We found the panic
in 2.6.32.43 is caused by changing cp->flags without protection. In
2.6.32.43, ip_vs_process_message changes cp->flags without protection
while update active/inactive flags for the connection.

After code inspiration, we found in 3.x kernel, it is accidentally
fixed by commit  f73181c. But with ip_vs_app changes,
ip_vs_ftp_init_conn() will have chance to change cp->flags without
protection. So it is a potential bug in 3.x kernel.


> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman July 17, 2012, 5:14 a.m. UTC | #3 
On Mon, Jul 16, 2012 at 11:07:57PM +0200, Pablo Neira Ayuso wrote:
> Hi Simon,
> 
> On Wed, Jul 11, 2012 at 09:19:22AM +0900, Simon Horman wrote:
> > From: Xiaotian Feng <xtfeng@gmail.com>
> > 
> > We met a kernel panic in 2.6.32.43 kernel:
> [...]
> >  net/netfilter/ipvs/ip_vs_ftp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> > index b20b29c..c2bc264 100644
> > --- a/net/netfilter/ipvs/ip_vs_ftp.c
> > +++ b/net/netfilter/ipvs/ip_vs_ftp.c
> > @@ -65,8 +65,10 @@ static int ip_vs_ftp_pasv;
> >  static int
> >  ip_vs_ftp_init_conn(struct ip_vs_app *app, struct ip_vs_conn *cp)
> >  {
> > +	spin_lock(&cp->lock);
> >  	/* We use connection tracking for the command connection */
> >  	cp->flags |= IP_VS_CONN_F_NFCT;
> > +	spin_unlock(&cp->lock);
> >  	return 0;
> 
> The conntrack support for FTP IPVS helper seems to be there since
> 2.6.37.
> 
> However, the patch description mentions 2.6.32.43.
> 
> Something doesn't match here, could you clarify this?

My understanding is that the problem was observed with 2.6.32
but that it has been around for much longer.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso July 17, 2012, 9:46 a.m. UTC | #4 
Hi,

On Tue, Jul 17, 2012 at 09:44:01AM +0800, Xiaotian Feng wrote:
> On Tue, Jul 17, 2012 at 5:07 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Simon,
> >
> > On Wed, Jul 11, 2012 at 09:19:22AM +0900, Simon Horman wrote:
> >> From: Xiaotian Feng <xtfeng@gmail.com>
> >>
> >> We met a kernel panic in 2.6.32.43 kernel:
> > [...]
> >>  net/netfilter/ipvs/ip_vs_ftp.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> >> index b20b29c..c2bc264 100644
> >> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> >> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
> >> @@ -65,8 +65,10 @@ static int ip_vs_ftp_pasv;
> >>  static int
> >>  ip_vs_ftp_init_conn(struct ip_vs_app *app, struct ip_vs_conn *cp)
> >>  {
> >> +     spin_lock(&cp->lock);
> >>       /* We use connection tracking for the command connection */
> >>       cp->flags |= IP_VS_CONN_F_NFCT;
> >> +     spin_unlock(&cp->lock);
> >>       return 0;
> >
> > The conntrack support for FTP IPVS helper seems to be there since
> > 2.6.37.
> >
> > However, the patch description mentions 2.6.32.43.
> >
> > Something doesn't match here, could you clarify this?
> >
> 
> Sorry for the misleading description in the patch. We found the panic
> in 2.6.32.43 is caused by changing cp->flags without protection. In
> 2.6.32.43, ip_vs_process_message changes cp->flags without protection
> while update active/inactive flags for the connection.
> 
> After code inspiration, we found in 3.x kernel, it is accidentally
> fixed by commit  f73181c. But with ip_vs_app changes,
> ip_vs_ftp_init_conn() will have chance to change cp->flags without
> protection. So it is a potential bug in 3.x kernel.

Please, then fix the patch description and resend the patch to me.

I have to justify why this is pushed forward to David, and using
misleading description for the patch is not the way to go.

Regarding this bitset operation, I think it's way better if you use
bitwise operations for those cp->flags. Getting the spin_lock just to
set the flag is way too much.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index b20b29c..c2bc264 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -65,8 +65,10 @@  static int ip_vs_ftp_pasv;
 static int
 ip_vs_ftp_init_conn(struct ip_vs_app *app, struct ip_vs_conn *cp)
 {
+	spin_lock(&cp->lock);
 	/* We use connection tracking for the command connection */
 	cp->flags |= IP_VS_CONN_F_NFCT;
+	spin_unlock(&cp->lock);
 	return 0;
 }