Re: [RFC davem] revert: net: Make skb->skb_iif always track skb->dev

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hello Jiri,

On 12.01.2013 19:13, Jiri Pirko wrote:

> Sat, Jan 12, 2013 at 02:48:14PM CET, socketcan@hartkopp.net wrote:
>> Hello Dave,
>>
>> in your below patch from 23 Jul 2012 you removed the check for an already set
>> value of skb_iif in net/core/dev.c
>>
>> I'm currently working on a solution to prevent some routed CAN frames to be
>> sent back onto the originating network device.
> 
> Hm, I'm not sure where exactly you want to use this information, but
> can_rcv() can get it from orig_dev->ifindex


No - it's not in can_rcv() ...

Depending on the filter lists in can_rcv() the received skb is passed to the
function can_can_gw_rcv() in net/can/gw.c.

An there i wanted to add this code to omit sending the CAN frame on the
originating interface:

@@ .. @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data

        if (!(gwj->dst.dev->flags & IFF_UP)) {
                gwj->dropped_frames++;
                return;
        }

+       /* is sending the skb back to the incoming interface allowed? */
+       if (!(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK) &&
+           skb->skb_iif == gwj->dst.dev->ifindex)
+               return;
+
        /*
         * clone the given skb, which has not been done in can_rcv()
         *

This works fine, when the patch from Dave is reverted.

I did not find any good solution to preserve the originating netdev over
several netif_receive_skb() calls - but this skb_iif which has been made
unusable ...

(..)

>>
>> To me it is not clear why skb_iff is needed anyway as the value should
>> always be available via skb->dev->ifindex, right?
>>
>> But if skb_iff has any right to exist it should contain the first incoming
>> interface on the host IMO.
>>
>> Please correct my if i'm wrong and/or tell me what your commit message means
>> in respect to my request and why skb->dev->ifindex is not used instead of
>> skb_iif. I feel somehow lost about the skb_iif intention ...
> 
> I believe that skb_iif should be removed.


AFAICS skb->skb_iif is used as some kind of cached variable for the original
skb->dev->ifindex - or is it really possible that skb->skb_iif is set while
skb->dev->ifindex is not accessible?

Btw. i my case skb_iif would make sense though.

Regards,
Oliver

>>
>> ---
>>
>>
>> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=b68581778cd0051a3fb9a2b614dee7eccb5127ff
>>
>> net: Make skb->skb_iif always track skb->dev
>>
>> Make it follow device decapsulation, from things such as VLAN and
>> bonding.
>>
>> The stuff that actually cares about pre-demuxed device pointers, is
>> handled by the "orig_dev" variable in __netif_receive_skb().  And
>> the only consumer of that is the po->origdev feature of AF_PACKET
>> sockets.
>>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> ---
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index cca02ae..0ebaea1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3173,8 +3173,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
>> 	if (netpoll_receive_skb(skb))
>> 		return NET_RX_DROP;
>>
>> -	if (!skb->skb_iif)
>> -		skb->skb_iif = skb->dev->ifindex;
>> 	orig_dev = skb->dev;
>>
>> 	skb_reset_network_header(skb);
>> @@ -3186,6 +3184,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
>> 	rcu_read_lock();
>>
>> another_round:
>> +	skb->skb_iif = skb->dev->ifindex;
>>
>> 	__this_cpu_inc(softnet_data.processed);
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html