Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
>> Hi Jan,
>>
>> Jan Kiszka wrote:
>>> [Somehow, my TB hates your mails - it just presents empty bodies to me
>>> when I hit reply. Strange.]
>>>
>>> Wolfgang Grandegger wrote:
>>>> This patch enables ARP support for the RT-Proxy Linux device.
>>>> Incoming ARP replys are delivered to both, the RTnet and the
>>>> Linux network stack. The RT-Proxy then gets attached to the
>>>> corresponding RTnet device, rteth0 by default. You can enable
>>>> this feature with the configure option "--enable-proxy-arp".
>>> Maybe the same question here: Do we need an extra knob for this, or are
>>> all proxy scenarios fine this enabled?
>>>
>>>> Note: this patch requires running "scripts/autogen.sh"
>>>>
>>>> Signed-off-by: Wolfang Grandegger <[EMAIL PROTECTED]>
>>>> Index: rtnet/stack/ipv4/arp.c
>>>> ===================================================================
>>>> --- rtnet.orig/stack/ipv4/arp.c
>>>> +++ rtnet/stack/ipv4/arp.c
>>>> @@ -25,6 +25,9 @@
>>>> #include <stack_mgr.h>
>>>> #include <ipv4/arp.h>
>>>>
>>>> +#ifdef CONFIG_RTNET_ADDON_PROXY_ARP
>>>> +#include <ipv4/ip_input.h>
>>>> +#endif /* CONFIG_RTNET_ADDON_PROXY_ARP */
>>>>
>>>> /***
>>>> * arp_send: Create and send an arp packet. If (dest_hw == NULL),
>>>> @@ -174,6 +177,12 @@ int rt_arp_rcv(struct rtskb *skb, struct
>>>> }
>>>>
>>>> out:
>>>> +#ifdef CONFIG_RTNET_ADDON_PROXY_ARP
>>>> + if (rt_ip_fallback_handler) {
>>>> + rt_ip_fallback_handler(skb);
>>>> + return 0;
>>>> + }
>>>> +#endif
>>> Hmm, I wonder what prevents ARP requests being answered twice: first by
>>> RTnet (a few lines above this hunk), and then potentially also by Linux.
>> I'm especially concerned about the way out-going packets are routed:
>>
>> #ifdef CONFIG_RTNET_ADDON_PROXY_ARP
>> rtdev_reference(rtnetproxy_rtdev);
>>
>> rtskb->rtdev = rtnetproxy_rtdev;
>>
>> /* Call the actual transmit function */
>> rtdev_xmit_proxy(rtskb);
>>
>> rtdev_dereference(rtnetproxy_rtdev);
>>
>> #else /* !CONFIG_RTNET_ADDON_PROXY_ARP */
>> /* Determine the device to use: Only ip routing is used here.
>> * Non-ip protocols are not supported... */
>> rc = rt_ip_route_output(&rt, pData->ip_dst, INADDR_ANY);
>> if (rc == 0)
>> {
>> struct rtnet_device *rtdev = rt.rtdev;
>> ...
>> }
>> ...
>> #endif /* CONFIG_RTNET_ADDON_PROXY_ARP */
>>
>> For ARP handled by Linux, I forward all out-going packets directly from
>> rtproxy to rteth0, or a device specified with the module parameter
>> rtdev_attach. Side note: as Gilles already mentioned, I would make sense
>> to have an rtproxy* for every rteth*. In the other case the out-going
>> packets are routed according to the RTnet routing table, which is a
>> different use-case. Therefore I tend to make this feature configurable.
>
> Yes, CONFIG_RTNET_ADDON_PROXY_ARP is in fact required to switch between
> two different modes - and also between two different rtproxy mapping. I
> would even say that it is mandatory to have one proxy device per rteth
> device if CONFIG_RTNET_ADDON_PROXY_ARP is on. That avoids the module
> parameters right from the start.
To be done. For the moment, we have just one proxy device.
>
>>>> kfree_rtskb(skb);
>>>> return 0;
>>>> }
>>>> Index: rtnet/addons/rtnetproxy.c
>>>> ===================================================================
>>>> --- rtnet.orig/addons/rtnetproxy.c
>>>> +++ rtnet/addons/rtnetproxy.c
>>>> @@ -105,6 +105,14 @@ static rtdm_task_t rtnetproxy_thread;
>>>>
>>>> static rtdm_sem_t rtnetproxy_sem;
>>>>
>>>> +#ifdef CONFIG_RTNET_ADDON_PROXY_ARP
>>>> +static char* rtdev_attach = "rteth0";
>>>> +module_param(rtdev_attach, charp, 0444);
>>>> +MODULE_PARM_DESC(rtdev_attach, "Attach to the specified RTnet device");
>>>> +
>>>> +struct rtnet_device *rtnetproxy_rtdev;
>>>> +#endif
>>>> +
>>>> /* ***********************************************************************
>>>> * Returns the next pointer from the ringbuffer or zero if nothing is
>>>> * available
>>>> @@ -181,7 +189,10 @@ static inline void send_data_out(struct
>>>> {
>>>>
>>>> struct rtskb *rtskb;
>>>> +#ifndef CONFIG_RTNET_ADDON_PROXY_ARP
>>>> struct dest_route rt;
>>>> + int rc;
>>>> +#endif
>>>>
>>>> struct skb_data_format
>>>> {
>>>> @@ -194,7 +205,6 @@ static inline void send_data_out(struct
>>>> * thus no spaces are allowed! */
>>>>
>>>> struct skb_data_format *pData;
>>>> - int rc;
>>>>
>>>> /* Copy the data from the standard sk_buff to the realtime sk_buff:
>>>> * Both have the same length. */
>>>> @@ -208,6 +218,18 @@ static inline void send_data_out(struct
>>>>
>>>> pData = (struct skb_data_format*) rtskb->data;
>>>>
>>>> +#ifdef CONFIG_RTNET_ADDON_PROXY_ARP
>>>> + rtskb->rtdev = rtnetproxy_rtdev;
>>>> +
>>>> + /* Call the actual transmit function */
>>>> + rtdev_xmit_proxy(rtskb);
>>>> +
>>>> + /* The rtskb is freed somewhere deep in the driver...
>>>> + * No need to do it here. */
>>>> +
>>>> + rtdev_dereference(rtskb->rtdev);
>>> I know this pattern is not new, but looking at it I wonder if this isn't
>>> generation a potential use-after-release (if xmitting causes an
>>> immediate rtskb release)...
>> First, there was a bug: rtdev_reference() was missing. The correct code
>> is shown above. What do you mean with "if xmitting causes an immediate
>> rtskb release"?
>
> OK, meanwhile I understood my own bad feelings about the hunk above: You
> must not access rtskb after handing it over to rtdev_xmit_proxy. But you
> also need not, just use rtnetproxy_rtdev.
OK.
Wolfgang.
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
RTnet-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/rtnet-users
