Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx> wrote on 10/26/2011 11:32:25
AM:
> >> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml
> >> b/examples/xml/nwfilter/no-ip-spoofing.xml
> >> index b8c94c8..e5969a0 100644
> >> --- a/examples/xml/nwfilter/no-ip-spoofing.xml
> >> +++ b/examples/xml/nwfilter/no-ip-spoofing.xml
> >> @@ -4,4 +4,9 @@
> >> <rule action='drop' direction='out'>
> >> <ip match='no' srcipaddr='$IP' />
> >> </rule>
> >> +<!-- allow DHCP requests -->
> >> +<rule action='return' direction='out'>
> >> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68'
> >> + srcportend='68' />
> >> +</rule>
> >> </filter>
> > Is this change necessary? Is it needed for the first instantiation of
> > the rules or is it needed to support the case when all IP addresses
> > have expired? I am asking because below I saw you calling
> The order of the two rules as given here is wrong. As long as the IP
> variable is not set, it will not generate the filter and once the
> variable is set it will never get to the 2nd rule.
I've removed this in the version I'm testing now. The DHCP-only
rules cover this without multiple address support. In the original, I
generated all chains and added and removed rules from those chains
dynamically
with address addition and removal (ie, it did not use
virNWFilterInstantiateLFilterLate()). I didn't see any ordering issues in
either version -- don't know what you mean there, but as you pointed
out, this change is unnecessary in the stripped-down single-address
version.
> Also, in case learning is NULL, you may need to set it to 'any' (for
> backwards-compatibility) to avoid a NULL pointer crash later on.
I didn't try it with NULL -- if it is not set (ie, the variable is not
present at all), it defaults to "any" already. If it's set to garbage,
that's an error case which I handle, unless I I've broken it in later
versions -- I'll retest this in v5, where I'm incorporating your other
comments now.
> In the
> other file there's an fclose(fp) that causes a crash (fp=NULL) in case
> the lease file was not available -> move the fclose() two lines up.
>
> Once the 2nd patch is applied and libvirt started, I see this here
>
> 2011-09-26 14:26:06.206: 21084: warning :
> networkAddGeneralIptablesRules:1270 : May need to update iptables
> package & kernel to support CHECKSUM rule.
> *** glibc detected *** /usr/sbin/libvirtd: malloc(): memory corruption:
> 0x00007fc5b433e010 ***
>
> This does not occur with only the first patch applied and only occurs if
> a lease file was found. So something in the leasefile support code is
> corrupting memory.
I didn't see any problems of this sort, with no pre-existing lease file or
an existing one with various leases, active, deleted and expired. I'll
look
at it some more and see if I can figure out what you're seeing.
> Also it probably should re-create the filters in case libvirt is
> restarted. Somehow you should be able to use the lease files to get the
> IP address to rebuild the filters. The 'IP learning' subsytem just did
> the static IP address detection in this case but with DHCP snooping one
> shouldn't have to cycle the interfaces to cause the DHCP protocol to
run.
Yes, that's the point of the lease file. I did only simple tests (but
including libvirtd restart) for this version (v4) since I don't think
anything
I changed affected this from previous revisions, but I'll stress it a
little
more and see if I can reproduce what you're seeing.
I did have some issues with restarting libvirtd (primarily VM's exiting
when
I restarted multiple times), but those were there before I applied any of
these patches; no log messages when that happened.
+-DLS
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list