LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable

To: Simon Horman <horms@xxxxxxxxxxxx>
Subject: Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable
Cc: Wensong Zhang <wensong@xxxxxxxxxxxx>, lvs-devel@xxxxxxxxxxxxxxx, Hans Schillstrom <hans@xxxxxxxxxxxxxxx>, Julian Anastasov <ja@xxxxxx>
From: Tinggong Wang <wangtinggong@xxxxxxxxx>
Date: Mon, 13 Dec 2010 11:44:38 +0800
on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@xxxxxxxxxxxx) wrote:
> [ CCed Hans Schillstrom and Julian Anastasov ]
> 
> On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > Signed-off-by: Tinggong Wang <wangtinggong@xxxxxxxxx>
> > ---
> >  net/netfilter/ipvs/ip_vs_sync.c |   13 ++++++++-----
> >  1 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c 
> > b/net/netfilter/ipvs/ip_vs_sync.c
> > index 7632a17..2b6b0cb 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, 
> > const size_t buflen)
> >     char *p;
> >     int i;
> >  
> > -   if (buflen < SYNC_MESG_HEADER_LEN) {
> > -           IP_VS_ERR_RL("sync message header too short\n");
> > -           return;
> > -   }
> > -
> >     /* Convert size back to host byte order */
> >     m->size = ntohs(m->size);
> >  
> > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> >                             break;
> >                     }
> >  
> > +                   /* throw invalid data before local_bh_disable,
> > +                    * so performance won't be downgraded by it
> > +                    */
> > +                   if (len < SYNC_MESG_HEADER_LEN) {
> > +                           IP_VS_ERR_RL("sync message header too short\n");
> > +                           continue;
> > +                   }
> > +
> >                     /* disable bottom half, because it accesses the data
> >                        shared by softirq while getting/creating conns */
> >                     local_bh_disable();
> > -- 
> > 1.7.2.3
> > 
> 
> Could you explain the motivation for this change?

in my opinion, before local_bh_disable, should ensure packets are look
like more resonable.

local_bh_disable will disable all bottom-half processing on local cpu,
if the multicast group flood of packets containing bad sync message,
local cpu will be busy doing local_bh_disable and local_bh_enable. 

if the backup pc has only one cpu, all other tasks will be pending until
the flood finished.

--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>