Re: [PATCH net-next 4/8] net: ipv6: mld: implement RFC3810 MLDv2 mode only

[Daniel Borkmann <dborkman@redhat.com>]

On 09/03/2013 11:00 PM, Hannes Frederic Sowa wrote:
> Hi Daniel!
>
> On Tue, Sep 03, 2013 at 09:59:34AM +0200, Daniel Borkmann wrote:
>> RFC3810, 10. Security Considerations says under subsection 10.1.
>> Query Message:
>>
>>    A forged Version 1 Query message will put MLDv2 listeners on that
>>    link in MLDv1 Host Compatibility Mode. This scenario can be avoided
>>    by providing MLDv2 hosts with a configuration option to ignore
>>    Version 1 messages completely.
>>
>> Hence, implement a MLDv2-only mode that will ignore MLDv1 traffic:
>>
>>    echo 2 > /proc/sys/net/ipv6/conf/ethX/force_mld_version
>
> I just played around with MLDv2-only mode and noticed that the commit message
> diverges from the code:
>
>> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
>> index 005b22f..02cd0c5 100644
>> --- a/net/ipv6/mcast.c
>> +++ b/net/ipv6/mcast.c
>> @@ -1112,9 +1112,21 @@ static bool mld_marksources(struct ifmcaddr6 *pmc, int nsrcs,
>>   	return true;
>>   }
>>
>> +static bool mld_in_v2_mode_only(const struct inet6_dev *idev)
>> +{
>> +	return dev_net(idev->dev)->ipv6.devconf_all->force_mld_version == 2;
>
> Maybe something like
>
> int val = idev->cnf.force_mld_version ?: dev_net(idev->dev)->ipv6.devconf_all->force_mld_version;
> return val == 2;

Hm, true, thanks for spotting. I think it makes sense, first check for individual idev
setting, then for whole namespace. I will update the series and send v2.

>> +}
>> +
>> +static bool mld_in_v1_mode_only(const struct inet6_dev *idev)
>> +{
>> +	return dev_net(idev->dev)->ipv6.devconf_all->force_mld_version == 1;
>
> Likewise.
>
>> +}
>> +
>>   static bool mld_in_v1_mode(const struct inet6_dev *idev)
>>   {
>> -	if (dev_net(idev->dev)->ipv6.devconf_all->force_mld_version == 1)
>> +	if (mld_in_v2_mode_only(idev))
>> +		return false;
>> +	if (mld_in_v1_mode_only(idev))
>>   		return true;
>>   	if (idev->cnf.force_mld_version == 1)
>>   		return true;
>
> This last if statement could be dropped then.
>
> Thanks,
>
>    Hannes
>