Re: [mpls-linux-general] bug: mpls/tc and mpls/netfilter, the same problem ?

[Olivier D. <Oli...@rd...>]

Hi Vincent,

Vincent Jardin wrote:
> 
> Find enclosed my patch in order to get mpls/tc that is working now ;-)))
> 
> The ingress filter was not working because:
> 1/ mpls_index was not set to the right value in sch_ingress.c

Why ? Can you tell me to what value mpls_index is set ? It's a parameter
pb. or a filter pb. ? Perhaps the label key must be preceed by 'Ox' like
with iptables to tell sch_ingress that the key is an hexadecimal value.

> 
> 2/ mpls_output needs to be called from ip_rcv_finish (ip_input.c) otherwise
> ip_route_input could not find the right entry and then the skb was delivered
> to ip_error().

No. If you do this, you strap many packet processing like header
verification, hearder option treatment, fragment ... If ip_route_input
doesn't find an entry in rt_hash_table, it call ip_route_input_slow
which compute a dst entry and insert it in the rt_hash_table for the
subsequent call.
We thought that we can call directly mpls_output from the netfilter
ipt_MPLS.c function, but it strap too much packet processing.

> 
> Moreover, I have tested my mpls/tc patch with the MPLS/netfilter patch. It
> works too ;-) If anyone wants to try it, you should add the following line
> into mpls_output.c:
> int mpls_output(struct sk_buff *skb) {
>   static const char *fn_name = "mpls_output";
>   struct mpls_push_data *mpr = (struct mpls_push_data*)kmem_cache_alloc(
>     mpls_mpr_cachep,GFP_ATOMIC);
> [... ]
>   mpr->bos = 1;
>   mpr->exp = 0;
> 
> #ifdef CONFIG_MPLS_INGRESS_POLICING
>   if (skb->mpls_index) {
>     unsigned int key = skb->mpls_index;
> 
>     MPLS_DEBUG(("%s: selecting moi with mpls_index 0x%x\n",fn_name,key));
>     RADIX_GET(&moi_tree,mpls_out_info_node,next,MPLS_TREE_BITS,unsigned int,
>       key,MPLS_TREE_DEPTH,moi,moi,retval);
>     if(retval || !moi) {
>       MPLS_DEBUG(("%s: unknown mpls_index\n",fn_name));
>       kmem_cache_free(mpls_mpr_cachep,mpr);
>       return -ESRCH;
>     }
> 
> mpls_again:
> +     skb->dst = moi->moi_dst; // XXX <- add this line

This is already done in mpls_output2 (line 100) which is call from
mpls_output, for the GEN label type only.

>     for(i=0;i<moi->moi_instruction_length;i++) {
>        switch(moi->moi_instruction[i].mi_opcode) {
>        case MPLS_OP_FWD:
> 
> [...]
> 
> Please Olivier, could you tell me if both work with your ATM boards too ? (I
> do not have any ;-( )
> 

We can make the test, but we find a big bug in our patch : the
skb->dst->pmtu is decrement by 4 for each call of mpls_output, so for
each packet, resulting in a negative pmtu after sending hundreds of
packets. We find a beginning of solution based on the FWMARK route
capability. We provide a new patch ASAP.

> Thanks,
>   Vincent
> 

Olivier
-- 
 FTR&D/DAC/CPN
 Technopole Anticipa     | mailto:Oli...@fr...
 2, Avenue Pierre Marzin | Phone:  +(33) 2 96 05 28 80
 F-22307 LANNION         | Fax:    +(33) 2 96 05 18 52