Re: [PATCH 09/14] can: raw: use struct sock::sk_bound_dev_if instead of struct raw_sock::ifindex

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 08/25/2017 08:46 PM, Kurt Van Dijck wrote:
> Hey Oliver,
> 
>> On 08/25/2017 11:34 AM, Kurt Van Dijck wrote:
>>>> On 08/24/2017 04:11 PM, Kurt Van Dijck wrote:
>>>>>>> This patch removes the struct raw_sock::ifindex from the CAN raw socket
>>>>>>> and uses the already existing sock::sk_bound_dev_if instead.
>>>>>>
>>>>>> I would like to omit this patch.
>>>>>>
>>>>>> 1. It does NOT increase the readability ;-)
>>>>>>
>>>>>> ifindex points out that we are handling an interface index.
>>>>>> sk_bound_dev_if could be anything.
>>>>>
>>>>> See http://elixir.free-electrons.com/linux/v4.12.8/source/net/core/sock.c#L617
>>>>> sk_bound_dev_if may be a difficult name, but could not be anything.
>>>>> It allow to use SO_BINDTODEVICE (man 7 socket) to work properly for can raw sockets too.
>>>>
>>>> When ro->ifindex = 0 it can still be bound in the case of CAN sockets.
>>>> There is a different semantic as ifindex = zero means 'all CAN interfaces'
>>>> and not 'no interface'.
>>>
>>> ro->ifindex = 0: accept traffic from all interfaces
>>> ro->ifindex != 0: accept traffic only from 1 iface
>>>
>>> sk_bound_dev_if = 0: accept traffic from all interfaces
>>> sk_bound_dev_if != 0: accept traffic only from 1 iface
>>>
>>> What is the semantic difference?
>>
>> man 7 socket
>>
>> SO_BINDTODEVICE
>>
>> Bind this socket to a particular device like “eth0”, as specified in the
>> passed interface name.  If the name  is  an empty  string  or the option
>> length is zero, the socket device binding is removed.  The passed option is
>> a variable-length null-terminated interface name string with the maximum
>> size of IFNAMSIZ.  If a socket is bound to  an  interface,  only  packets
>> received from that particular interface are processed by the socket. Note
>> that this works only for some socket types, particularly AF_INET sockets.
>> It is not supported for packet  sockets  (use  normal bind(2) there).
> 
> A verbatim copy of the man page isn't helping. I have read it.
> 
>>
>> Especially "If the name  is  an empty  string  or the option length is zero,
>> the socket device binding is removed." is wrong.
> 
> Do you mean the man page is wrong?
> Or do you mean that the semantic difference is wrong?

Yes. It tells us:

"If the name  is  an empty  string  or the option length is zero, the 
socket device binding is removed."

Which means the status of 'bind' is NOT decoupled from the number in 
ifindex.

A provided empty string leads to an unbound socket - and not to a socket 
bound to ifindex=0 which is intended with CAN sockets.

> Or ?
> 
>>
>> A bound CAN_RAW socket can have ifindex = 0.
> 
> I see no reason why a bound UDP socket can not have sk_bound_dev_if = 0.
> I see no reason why a bound CAN_RAW socket can not either.
> I start to think here that the word 'device binding' confuses you with
> bind(2), but I have not seen any reference that SO_BINDTODEVICE
> conflicts/interacts with bind() so far.
> 
>>
>> And "It is not supported for packet  sockets  (use  normal  bind(2) there)"
>> - the CAN_RAW socket is based on PF_PACKET sockets with the device binding
>> and addressing concept.
>>
>> The more I look into this, the more I'm sure that we should leave it as-is.
> 
>>>>>> I would like to omit this patch.
> 
> Well, I feel it may block the others, so indeed, omitting may be the
> best by lack of consensus.

Ok. We should omit this patch.

But I'm more interested in convincing you than just omitting this patch 
to not block the upstream process ;-)

Best regards,
Oliver