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.
>
>>> 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.
Jan
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------- 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 RTnet-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/rtnet-users
