Re: [libvirt] [libvirt PATCHv4 1/2] add DHCP snooping
Stefan Berger <stefanb(a)linux.vnet.ibm.com> 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