subject:"RFC\: L2TPv3 interface"

Re: RFC: L2TPv3 interface

2017-02-16 Thread Kengo NAKAHARA

Hi,

On 2017/02/07 14:01, Kengo NAKAHARA wrote:
> On 2017/01/20 21:26, Kengo NAKAHARA wrote:
>> At first, here is updated patches.
>>
>> http://netbsd.org/~knakahara/if-l2tp-2/01-accept-ifname-include-digit.patch
>>http://netbsd.org/~knakahara/if-l2tp-2/02-if-l2tp.patch
> 
> I rebase and add some fixes. Here is updated patch set and unified patch.
> - patch set
>   http://netbsd.org/~knakahara/if-l2tp-3/if-l2tp-3.tgz
> - unified patch
>   http://netbsd.org/~knakahara/if-l2tp-3/if-l2tp-3.patch
> 
> Could you comment this patch set? If there is no objection, I will commit
> this patch set after a few days or weeks.

I committed it.


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA

Re: RFC: L2TPv3 interface

2017-02-06 Thread Kengo NAKAHARA

Hi,

On 2017/01/20 21:26, Kengo NAKAHARA wrote:
> At first, here is updated patches.
>http://netbsd.org/~knakahara/if-l2tp-2/01-accept-ifname-include-digit.patch
>http://netbsd.org/~knakahara/if-l2tp-2/02-if-l2tp.patch

I rebase and add some fixes. Here is updated patch set and unified patch.
- patch set
  http://netbsd.org/~knakahara/if-l2tp-3/if-l2tp-3.tgz
- unified patch
  http://netbsd.org/~knakahara/if-l2tp-3/if-l2tp-3.patch

Could you comment this patch set? If there is no objection, I will commit
this patch set after a few days or weeks.


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA

Re: RFC: L2TPv3 interface

2017-01-22 Thread Kengo NAKAHARA

Hi,


On 2017/01/20 21:26, Kengo NAKAHARA wrote:
> On 2017/01/20 0:38, Taylor R Campbell wrote:
>>Date: Thu, 19 Jan 2017 17:58:17 +0900
>>From: Kengo NAKAHARA 
>>+/*
>>+ * l2tp_variant update API.
>>+ *
>>+ * Assumption:
>>+ * reader side dereferences sc->l2tp_var in reader critical section only,
>>+ * that is, all of reader sides do not reader the sc->l2tp_var after
>>+ * pserialize_perform().
>>+ */
>>+static void
>>+l2tp_variant_update(struct l2tp_softc *sc, struct l2tp_variant *nvar)
>>+{
>>+   struct ifnet *ifp = &sc->l2tp_ec.ec_if;
>>+   struct l2tp_variant *ovar = sc->l2tp_var;
>>+
>>+   KASSERT(mutex_owned(&sc->l2tp_lock));
>>+
>>+   membar_producer();
>>+   atomic_swap_ptr(&sc->l2tp_var, nvar);
>>+   pserialize_perform(l2tp_psz);
>>+   psref_target_destroy(&ovar->lv_psref, lv_psref_class);
>>
>> No need for atomic_swap_ptr.  Just
>>
>> sc->l2tp_var = nvar;
>>
>> is enough.  Nobody else can write to it because we hold the lock.
> 
> Between writer and writer, it is correct. However, between writer and
> reader, I think atomic_swap_ptr is required to prevent reader's load
> before writer's store done. Is this correct?

Sorry, I was wrong. Reader has nothing to do with atomic_ops. So,
As you said, atomic_swap_ptr is not required here. I will fix it.


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA

Re: RFC: L2TPv3 interface

2017-01-20 Thread Joerg Sonnenberger

On Fri, Jan 20, 2017 at 09:26:52PM +0900, Kengo NAKAHARA wrote:
> On 2017/01/20 0:38, Taylor R Campbell wrote:
> >Date: Thu, 19 Jan 2017 17:58:17 +0900
> >From: Kengo NAKAHARA 
> > A few little comments:
> > 
> >diff --git a/sys/net/if.c b/sys/net/if.c
> >index 2386af3..ba63266 100644
> >--- a/sys/net/if.c
> >+++ b/sys/net/if.c
> >@@ -1599,7 +1613,7 @@ if_clone_lookup(const char *name, int *unitp)
> >strcpy(ifname, "if_");
> >/* separate interface name from unit */
> >for (dp = ifname + 3, cp = name; cp - name < IFNAMSIZ &&
> >-   *cp && (*cp < '0' || *cp > '9');)
> >+   *cp && !if_is_unit(cp);)
> >*dp++ = *cp++;
> > 
> > This changes the generic syntax interface names, perhaps to allow the
> > `2' in `l2tp', although since this loop skips over the first three
> > octets that doesn't seem to be necessary.  Either way, I don't have a
> > problem with this, but it should be done in a separate change.
> 
> I see. But sorry, I want to postpone the fix to reduce unnecessary
> skip... As a first step, I separate this changes and will commit first.

BTW, a proper fix should be looking backwards from the end of the
string and not forward.

Joerg

Re: RFC: L2TPv3 interface

2017-01-20 Thread Kengo NAKAHARA

Hi riastradh@n.o,

Thank you for your detailed review!


At first, here is updated patches.
   http://netbsd.org/~knakahara/if-l2tp-2/01-accept-ifname-include-digit.patch
   http://netbsd.org/~knakahara/if-l2tp-2/02-if-l2tp.patch

And then, each response is below.

On 2017/01/20 0:38, Taylor R Campbell wrote:
>Date: Thu, 19 Jan 2017 17:58:17 +0900
>From: Kengo NAKAHARA 
> A few little comments:
> 
>diff --git a/sys/net/if.c b/sys/net/if.c
>index 2386af3..ba63266 100644
>--- a/sys/net/if.c
>+++ b/sys/net/if.c
>@@ -1599,7 +1613,7 @@ if_clone_lookup(const char *name, int *unitp)
>strcpy(ifname, "if_");
>/* separate interface name from unit */
>for (dp = ifname + 3, cp = name; cp - name < IFNAMSIZ &&
>-   *cp && (*cp < '0' || *cp > '9');)
>+   *cp && !if_is_unit(cp);)
>*dp++ = *cp++;
> 
> This changes the generic syntax interface names, perhaps to allow the
> `2' in `l2tp', although since this loop skips over the first three
> octets that doesn't seem to be necessary.  Either way, I don't have a
> problem with this, but it should be done in a separate change.

I see. But sorry, I want to postpone the fix to reduce unnecessary
skip... As a first step, I separate this changes and will commit first.


>diff --git a/sys/net/if_l2tp.c b/sys/net/if_l2tp.c
>new file mode 100644
>index 000..dda8bbd
>--- /dev/null
>+++ b/sys/net/if_l2tp.c
>@@ -0,0 +1,1541 @@
>[...]
>+/*
>+ * l2tp global variable definitions
>+ */
>+LIST_HEAD(l2tp_sclist, l2tp_softc);
>+static struct l2tp_sclist l2tp_softc_list;
>+kmutex_t l2tp_list_lock;
>+
>+#if !defined(L2TP_ID_HASH_SIZE)
>+#define L2TP_ID_HASH_SIZE 64
>+#endif
>+static u_long l2tp_id_hash_mask;
>+
>+kmutex_t l2tp_hash_lock;
>+static struct pslist_head *l2tp_hashed_list = NULL;
> 
> Consider putting related global state into cacheline-aligned structs?

Oh, I forgot it. I should put them into cacheline-aligned structs.
In addition, I remove l2tp_id_hash_mask variable and use L2TP_ID_HASH_SIZE
to avoid holding lock in fast-path (l2tp_lookup_session_ref() =>
id_hash_func()).


> static struct {
> kmutex_t  lock;
> struct l2tp_sclistlist;
> } l2tp_softc __cacheline_aligned;
> 
> static struct {
> kmutex_t  lock;
> struct pslist_head*list;
>   unsigned long   mask;
> } l2tp_hash __cacheline_aligned;
> 
>+pserialize_t l2tp_psz;
>+struct psref_class *lv_psref_class __read_mostly;
> 
> __read_mostly for l2tp_psz?

Yes, I add it.

>+static int
>+l2tpdetach(void)
>+{
>+   int error;
>+
>+   if (!LIST_EMPTY(&l2tp_softc_list))
>+   return EBUSY;
> 
> Need lock here?  Need to first set flag preventing new creation?
> 
> mutex_enter(&l2tp_softc.lock);
> KASSERT(!l2tp_softc.dying);
> l2tp_softc.detaching = true;
> if (!LIST_EMPTY(&l2tp_softc.list)) {
>   l2tp_softc.detaching = false;
>   mutex_exit(&l2tp_softc.lock);
>   return EBUSY;
> }
> mutex_exit(&l2tp_softc.lock);
> 
> Anyone trying to add to l2tp_softc.list must also check
> l2tp_softc.detaching before proceeding.

You are right. Hmm..., it's seems there are same problems in
other module'd interfaces such as pppoe(4), gre(4), and so on.
I think module framework could fix this problem, so I will ask
pgoyette@n.o and christos@n.o if they have any idea later.


>+static int
>+l2tp_clone_destroy(struct ifnet *ifp)
>+{
>+   struct l2tp_softc *sc = (void *) ifp;
> 
> Use container_of here:
> 
>   struct l2tp_softc *sc = container_of(ifp, struct l2tp_softc,
>   l2tp_ec.ec_if);
> 
> No functional difference, but the compiler type-checks it.

I use container_of here and similar codes.

>+void
>+l2tp_input(struct mbuf *m, struct ifnet *ifp)
>+{
>+
>+   KASSERT(ifp != NULL);
>+
>+   if (0 == (mtod(m, u_long) & 0x03)) {
>+   /* copy and align head of payload */
>+   struct mbuf *m_head;
>+   int copy_length;
>+
>+#define L2TP_COPY_LENGTH   60
>+#define L2TP_LINK_HDR_ROOM (MHLEN - L2TP_COPY_LENGTH - 4/*round4(2)*/)
>+
>+   if (m->m_pkthdr.len < L2TP_COPY_LENGTH) {
>+   copy_length = m->m_pkthdr.len;
>+   } else {
>+   copy_length = L2TP_COPY_LENGTH;
>+   }
>+
>+   if (m->m_len < copy_length) {
>+   m = m_pullup(m, copy_length);
>+   if (m == NULL)
>+   return;
>+   }
>+
>+   MGETHDR(m_head, M_DONTWAIT, MT_HEADER);
>+   if (m_head == NULL) {
>+   m_freem(m);
>+   return;
>+

Re: RFC: L2TPv3 interface

2017-01-19 Thread Taylor R Campbell

   Date: Thu, 19 Jan 2017 17:58:17 +0900
   From: Kengo NAKAHARA 

   My co-workers implemented L2TPv3(RFC3931) interface for old version
   NetBSD.  And then, I port the inteface to NetBSD-current and
   MP-ify. Here is the patch.

   http://netbsd.org/~knakahara/if-l2tp/if-l2tp.patch

Cool!

A few little comments:

   diff --git a/sys/net/if.c b/sys/net/if.c
   index 2386af3..ba63266 100644
   --- a/sys/net/if.c
   +++ b/sys/net/if.c
   @@ -1599,7 +1613,7 @@ if_clone_lookup(const char *name, int *unitp)
   strcpy(ifname, "if_");
   /* separate interface name from unit */
   for (dp = ifname + 3, cp = name; cp - name < IFNAMSIZ &&
   -   *cp && (*cp < '0' || *cp > '9');)
   +   *cp && !if_is_unit(cp);)
   *dp++ = *cp++;

This changes the generic syntax interface names, perhaps to allow the
`2' in `l2tp', although since this loop skips over the first three
octets that doesn't seem to be necessary.  Either way, I don't have a
problem with this, but it should be done in a separate change.
 
   diff --git a/sys/net/if_l2tp.c b/sys/net/if_l2tp.c
   new file mode 100644
   index 000..dda8bbd
   --- /dev/null
   +++ b/sys/net/if_l2tp.c
   @@ -0,0 +1,1541 @@
   [...]
   +/*
   + * l2tp global variable definitions
   + */
   +LIST_HEAD(l2tp_sclist, l2tp_softc);
   +static struct l2tp_sclist l2tp_softc_list;
   +kmutex_t l2tp_list_lock;
   +
   +#if !defined(L2TP_ID_HASH_SIZE)
   +#define L2TP_ID_HASH_SIZE 64
   +#endif
   +static u_long l2tp_id_hash_mask;
   +
   +kmutex_t l2tp_hash_lock;
   +static struct pslist_head *l2tp_hashed_list = NULL;

Consider putting related global state into cacheline-aligned structs?

static struct {
kmutex_tlock;
struct l2tp_sclist  list;
} l2tp_softc __cacheline_aligned;

static struct {
kmutex_tlock;
struct pslist_head  *list;
unsigned long   mask;
} l2tp_hash __cacheline_aligned;

   +pserialize_t l2tp_psz;
   +struct psref_class *lv_psref_class __read_mostly;

__read_mostly for l2tp_psz?

   +static int
   +l2tpdetach(void)
   +{
   +   int error;
   +
   +   if (!LIST_EMPTY(&l2tp_softc_list))
   +   return EBUSY;

Need lock here?  Need to first set flag preventing new creation?

mutex_enter(&l2tp_softc.lock);
KASSERT(!l2tp_softc.dying);
l2tp_softc.detaching = true;
if (!LIST_EMPTY(&l2tp_softc.list)) {
l2tp_softc.detaching = false;
mutex_exit(&l2tp_softc.lock);
return EBUSY;
}
mutex_exit(&l2tp_softc.lock);

Anyone trying to add to l2tp_softc.list must also check
l2tp_softc.detaching before proceeding.

   +static int
   +l2tp_clone_destroy(struct ifnet *ifp)
   +{
   +   struct l2tp_softc *sc = (void *) ifp;

Use container_of here:

struct l2tp_softc *sc = container_of(ifp, struct l2tp_softc,
l2tp_ec.ec_if);

No functional difference, but the compiler type-checks it.

   +static int
   +l2tp_output(struct ifnet *ifp, struct mbuf *m, const struct sockaddr *dst,
   +const struct rtentry *rt)
   +{
   +   struct l2tp_softc *sc = (struct l2tp_softc*)ifp;

container_of

   +void
   +l2tp_input(struct mbuf *m, struct ifnet *ifp)
   +{
   +
   +   KASSERT(ifp != NULL);
   +
   +   if (0 == (mtod(m, u_long) & 0x03)) {
   +   /* copy and align head of payload */
   +   struct mbuf *m_head;
   +   int copy_length;
   +
   +#define L2TP_COPY_LENGTH   60
   +#define L2TP_LINK_HDR_ROOM (MHLEN - L2TP_COPY_LENGTH - 4/*round4(2)*/)
   +
   +   if (m->m_pkthdr.len < L2TP_COPY_LENGTH) {
   +   copy_length = m->m_pkthdr.len;
   +   } else {
   +   copy_length = L2TP_COPY_LENGTH;
   +   }
   +
   +   if (m->m_len < copy_length) {
   +   m = m_pullup(m, copy_length);
   +   if (m == NULL)
   +   return;
   +   }
   +
   +   MGETHDR(m_head, M_DONTWAIT, MT_HEADER);
   +   if (m_head == NULL) {
   +   m_freem(m);
   +   return;
   +   }
   +   M_COPY_PKTHDR(m_head, m);
   +
   +   m_head->m_data += 2 /* align */ + L2TP_LINK_HDR_ROOM;
   +   memcpy(m_head->m_data, m->m_data, copy_length);
   +   m_head->m_len = copy_length;
   +   m->m_data += copy_length;
   +   m->m_len -= copy_length;
   +
   +   /* construct chain */
   +   if (m->m_len == 0) {
   +   m_head->m_next = m_free(m); /* not m_freem */
   +   } else {
   +   /*
   +* copyed mtag in previous call M_COPY_PKTHDR
   +* but don't delete mtag in case cutt of M_PKTHDR 
flag
   +*/
   +   m

Re: RFC: L2TPv3 interface

2017-01-19 Thread Christos Zoulas

In article <93c56002-6eb8-0694-3b68-20805ed07...@iij.ad.jp>,
Kengo NAKAHARA   wrote:
>Hi,
>
>My co-workers implemented L2TPv3(RFC3931) interface for old version NetBSD.
>And then, I port the inteface to NetBSD-current and MP-ify. Here is the patch.
>http://netbsd.org/~knakahara/if-l2tp/if-l2tp.patch
>
>The L2TPv3 interface implementation is named l2tp(4), which can send/receive
>L2TPv3 data packet and control packet. Above patch include ifconfig(8)
>extension, so we can create L2TPv3 tunnel between hostA and hostB as below.

LGTM!

christos

RFC: L2TPv3 interface

2017-01-19 Thread Kengo NAKAHARA

Hi,

My co-workers implemented L2TPv3(RFC3931) interface for old version NetBSD.
And then, I port the inteface to NetBSD-current and MP-ify. Here is the patch.
http://netbsd.org/~knakahara/if-l2tp/if-l2tp.patch

The L2TPv3 interface implementation is named l2tp(4), which can send/receive
L2TPv3 data packet and control packet. Above patch include ifconfig(8)
extension, so we can create L2TPv3 tunnel between hostA and hostB as below.
== host A ==
# ifconfig wm0 inet 192.168.0.1/24
# ifconfig l2tp0 create
# ifconfig l2tp0 tunnel 192.168.0.1 192.168.0.2
# ifconfig l2tp0 session 1234 4321
# ifconfig bridge0 create
# brconfig bridge0 add wm1
# brconfig bridge0 add l2tp0
# ifconfig l2tp0 up
# ifconfig wm1 up
# ifconfig bridge0 up
== host A ==

== host B ==
# ifconfig l2tp0 create
# ifconfig l2tp0 tunnel 192.168.0.2 192.168.0.1
# ifconfig l2tp0 session 4321 1234
# ifconfig bridge0 create
# brconfig bridge0 add wm1
# brconfig bridge0 add l2tp0
# ifconfig l2tp0 up
# ifconfig wm1 up
# ifconfig bridge0 up
== host B ==

As l2tp(4) interface can send/receive L2TPv3 control packet, we can implement
userland daemon which manage sessions, that is, authentication, keepalive, and
so on. That's future work.

Could you comment this patch?


Thanks,

-- 
//
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Kengo NAKAHARA

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

Re: RFC: L2TPv3 interface

RFC: L2TPv3 interface

8 matches

Site Navigation

Mail list logo

Footer information