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

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 [off-list ref] wrote:

quoted

Hi Simon,

On Wed, Jul 11, 2012 at 09:19:22AM +0900, Simon Horman wrote:

quoted

From: Xiaotian Feng <redacted>

We met a kernel panic in 2.6.32.43 kernel:

[...]

quoted

 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.