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