LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

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

To: Hans Schillstrom <hans.schillstrom@xxxxxxxxxxxx>
Subject: Re: [PATCH 2/3] ipvs: check data validation before local_bh_disable
Cc: Simon Horman <horms@xxxxxxxxxxxx>, Wensong Zhang <wensong@xxxxxxxxxxxx>, "lvs-devel@xxxxxxxxxxxxxxx" <lvs-devel@xxxxxxxxxxxxxxx>, Hans Schillstrom <hans@xxxxxxxxxxxxxxx>, Julian Anastasov <ja@xxxxxx>
From: Tinggong Wang <wangtinggong@xxxxxxxxx>
Date: Mon, 13 Dec 2010 18:49:11 +0800
on Mon, 13 Dec 2010 09:53:01AM +0100 Hans Schillstrom 
(hans.schillstrom@xxxxxxxxxxxx) wrote:
> On Mon, 2010-12-13 at 07:29 +0100, Simon Horman wrote:
> > On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> > > 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.
> > 
> > Ok, that does sound reasonable to some extent. But realistically
> > this should only occur if bogus packets are being sent. And in
> > that case it would be possible for bogus packets to be more carefully
> > crafted such that we need to enter ip_vs_process_message() anyway.
> > So I'm not sure if there really is a gain here.
> > 
> I do agree, first of all It's a multicast and they are never opened in
> firewall so who should flood us? 
> (If IPVS addr and port is open close it)
> I don't think the extra rows actually adds anything as you say.
> 
Yes, it has small possibility to occur. and this patch only make sense
when the bogus packets length less than SYNC_MESG_HEADER_LEN.

but if it occurs, for example, someone write a program, join the
multicast group cursorily, and floods bogus packets accidentally.
backup's performace will be downgraded.

is this scenario should be included? if so, i'll try to improve this
patch. 

Thanks!
--
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>