[PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Taehee Yoo]

rhashtable_lookup() can return NULL. so that NULL pointer
check routine should be added.

Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/core/xdp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 9d1f220..1c12bc7 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		rcu_read_lock();
 		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
 		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-		xa->zc_alloc->free(xa->zc_alloc, handle);
+		if (xa)
+			xa->zc_alloc->free(xa->zc_alloc, handle);
 		rcu_read_unlock();
 	default:
 		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */
-- 
2.9.3

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Martin KaFai Lau]

On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> rhashtable_lookup() can return NULL. so that NULL pointer
> check routine should be added.
> 
> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
>  net/core/xdp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9d1f220..1c12bc7 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>  		rcu_read_lock();
>  		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>  		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> -		xa->zc_alloc->free(xa->zc_alloc, handle);
> +		if (xa)
> +			xa->zc_alloc->free(xa->zc_alloc, handle);
hmm...It is not clear to me the "!xa" case don't have to be handled?

>  		rcu_read_unlock();
>  	default:
>  		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */
> -- 
> 2.9.3
> 

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Jakub Kicinski]

On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:
> On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> > rhashtable_lookup() can return NULL. so that NULL pointer
> > check routine should be added.
> > 
> > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> >  net/core/xdp.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 9d1f220..1c12bc7 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> >  		rcu_read_lock();
> >  		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> >  		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > -		xa->zc_alloc->free(xa->zc_alloc, handle);
> > +		if (xa)
> > +			xa->zc_alloc->free(xa->zc_alloc, handle);  
> hmm...It is not clear to me the "!xa" case don't have to be handled?

Actually I have a more fundamental question about this interface I've
been meaning to ask.  

IIUC free() can happen on any CPU at any time, when whatever device,
socket or CPU this got redirected to completed the TX.  IOW there may
be multiple producers.  Drivers would need to create spin lock a'la the
a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?

We need some form of internal kernel circulation which would be MPSC.
I'm currently hacking up the XSK code to tell me whether the frame was
consumed by the correct XSK, and always clone the frame otherwise
(claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).

I feel like I'm missing something about the code.  Is redirect of
ZC/UMEM frame outside the xsk not possible and the only returns we will
see are from net/xdp/xsk.c?  That would work, but I don't see such a
check.  Help would be appreciated.

Also the fact that XSK bufs can't be freed, only completed, adds to the
pain of implementing AF_XDP, we'd certainly need some form of "give
back the frame, but I may need it later" SPSC mechanism, otherwise
driver writers will have tough time.  Unless, again, I'm missing
something about the code :)

> >  		rcu_read_unlock();
> >  	default:
> >  		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Taehee Yoo]

2018-07-21 2:18 GMT+09:00 Martin KaFai Lau <kafai@fb.com>:
> On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
>> rhashtable_lookup() can return NULL. so that NULL pointer
>> check routine should be added.
>>
>> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
>> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>> ---
>>  net/core/xdp.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 9d1f220..1c12bc7 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>>               rcu_read_lock();
>>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>> -             xa->zc_alloc->free(xa->zc_alloc, handle);
>> +             if (xa)
>> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
> hmm...It is not clear to me the "!xa" case don't have to be handled?

Thank you for reviewing!

Returning NULL pointer is bug case such as calling after use
xdp_rxq_info_unreg().
so that, I think it can't handle at that moment.
we can make __xdp_return to add WARN_ON_ONCE() or
add return error code to driver.
But I'm not sure if these is useful information.

I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
because there is no use case of MEM_TYPE_ZERO_COPY yet.

Thanks!

>
>>               rcu_read_unlock();
>>       default:
>>               /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
>> --
>> 2.9.3
>>

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Björn Töpel]

Den fre 20 juli 2018 kl 22:08 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:
> > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> > > rhashtable_lookup() can return NULL. so that NULL pointer
> > > check routine should be added.
> > >
> > > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > ---
> > >  net/core/xdp.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > index 9d1f220..1c12bc7 100644
> > > --- a/net/core/xdp.c
> > > +++ b/net/core/xdp.c
> > > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > >             rcu_read_lock();
> > >             /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > >             xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > -           xa->zc_alloc->free(xa->zc_alloc, handle);
> > > +           if (xa)
> > > +                   xa->zc_alloc->free(xa->zc_alloc, handle);
> > hmm...It is not clear to me the "!xa" case don't have to be handled?
>
> Actually I have a more fundamental question about this interface I've
> been meaning to ask.
>
> IIUC free() can happen on any CPU at any time, when whatever device,
> socket or CPU this got redirected to completed the TX.  IOW there may
> be multiple producers.  Drivers would need to create spin lock a'la the
> a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?
>

Jakub, apologies for the slow response. I'm still in
"holiday/hammock&beer mode", but will be back in a week. :-P

The idea with the xdp_return_* functions are that an xdp_buff and
xdp_frame can have custom allocations schemes. The difference beween
struct xdp_buff and struct xdp_frame is lifetime. The xdp_buff
lifetime is within the napi context, whereas xdp_frame can have a
lifetime longer/outside the napi context. E.g. for a XDP_REDIRECT
scenario an xdp_buff is converted to a xdp_frame. The conversion is
done in include/net/xdp.h:convert_to_xdp_frame.

Currently, the zero-copy MEM_TYPE_ZERO_COPY memtype can *only* be used
for xdp_buff, meaning that the lifetime is constrained to a napi
context. Further, given an xdp_buff with memtype MEM_TYPE_ZERO_COPY,
doing XDP_REDIRECT to a target that is *not* an AF_XDP socket would
mean converting the xdp_buff to an xdp_frame. The xdp_frame can then
be free'd on any CPU.

Note that the xsk_rcv* functions is always called from an napi
context, and therefore is using the xdp_return_buff calls.

To answer your question -- no, this fix is *not* needed, because the
xdp_buff napi constrained, and the xdp_buff will only be free'd on one
CPU.

> We need some form of internal kernel circulation which would be MPSC.
> I'm currently hacking up the XSK code to tell me whether the frame was
> consumed by the correct XSK, and always clone the frame otherwise
> (claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).
>
> I feel like I'm missing something about the code.  Is redirect of
> ZC/UMEM frame outside the xsk not possible and the only returns we will
> see are from net/xdp/xsk.c?  That would work, but I don't see such a
> check.  Help would be appreciated.
>

Right now, this is the case (refer to the TODO in
convert_to_xdp_frame), i.e. you cannot redirect an ZC/UMEM allocated
xdp_buff to a target that is not an xsk. This must, obviously, change
so that an xdp_buff (of MEM_TYPE_ZERO_COPY) can be converted to an
xdp_frame. The xdp_frame must be able to be free'd from multiple CPUs,
so here the a more sophisticated allocation scheme is required.

> Also the fact that XSK bufs can't be freed, only completed, adds to the
> pain of implementing AF_XDP, we'd certainly need some form of "give
> back the frame, but I may need it later" SPSC mechanism, otherwise
> driver writers will have tough time.  Unless, again, I'm missing
> something about the code :)
>

Yup, moving the recycling scheme from driver to "generic" is a good
idea! I need to finish up those i40e zerocopy patches first though...

(...and I'm very excited that you're doing nfp support for AF_XDP!!!)


Björn

> > >             rcu_read_unlock();
> > >     default:
> > >             /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
>

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Björn Töpel]

Den lör 21 juli 2018 kl 14:58 skrev Taehee Yoo <ap420073@gmail.com>:
>
> 2018-07-21 2:18 GMT+09:00 Martin KaFai Lau <kafai@fb.com>:
> > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> >> rhashtable_lookup() can return NULL. so that NULL pointer
> >> check routine should be added.
> >>
> >> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> >> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> >> ---
> >>  net/core/xdp.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
> >> index 9d1f220..1c12bc7 100644
> >> --- a/net/core/xdp.c
> >> +++ b/net/core/xdp.c
> >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> >>               rcu_read_lock();
> >>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
> >> +             if (xa)
> >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
> > hmm...It is not clear to me the "!xa" case don't have to be handled?
>
> Thank you for reviewing!
>
> Returning NULL pointer is bug case such as calling after use
> xdp_rxq_info_unreg().
> so that, I think it can't handle at that moment.
> we can make __xdp_return to add WARN_ON_ONCE() or
> add return error code to driver.
> But I'm not sure if these is useful information.
>
> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
> because there is no use case of MEM_TYPE_ZERO_COPY yet.
>

Taehee, again, sorry for the slow response and thanks for patch!

If xa is NULL, the driver has a buggy/broken implementation. What
would be a proper way of dealing with this? BUG?


Björn

> Thanks!
>
> >
> >>               rcu_read_unlock();
> >>       default:
> >>               /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
> >> --
> >> 2.9.3
> >>

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Taehee Yoo]

2018-07-23 18:41 GMT+09:00 Björn Töpel <bjorn.topel@gmail.com>:
> Den lör 21 juli 2018 kl 14:58 skrev Taehee Yoo <ap420073@gmail.com>:
>>
>> 2018-07-21 2:18 GMT+09:00 Martin KaFai Lau <kafai@fb.com>:
>> > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
>> >> rhashtable_lookup() can return NULL. so that NULL pointer
>> >> check routine should be added.
>> >>
>> >> Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
>> >> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>> >> ---
>> >>  net/core/xdp.c | 3 ++-
>> >>  1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> >> index 9d1f220..1c12bc7 100644
>> >> --- a/net/core/xdp.c
>> >> +++ b/net/core/xdp.c
>> >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>> >>               rcu_read_lock();
>> >>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>> >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>> >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
>> >> +             if (xa)
>> >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
>> > hmm...It is not clear to me the "!xa" case don't have to be handled?
>>
>> Thank you for reviewing!
>>
>> Returning NULL pointer is bug case such as calling after use
>> xdp_rxq_info_unreg().
>> so that, I think it can't handle at that moment.
>> we can make __xdp_return to add WARN_ON_ONCE() or
>> add return error code to driver.
>> But I'm not sure if these is useful information.
>>
>> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
>> because there is no use case of MEM_TYPE_ZERO_COPY yet.
>>
>
> Taehee, again, sorry for the slow response and thanks for patch!
>
> If xa is NULL, the driver has a buggy/broken implementation. What
> would be a proper way of dealing with this? BUG?
>

Thank you for reviewing!

I would like to add WARN_ON_ONCE. because code writers can
get opportunity for debugging this in runtime. also I think this bug
doesn't make critical side effect.

Thanks!

>
> Björn
>
>> Thanks!
>>
>> >
>> >>               rcu_read_unlock();
>> >>       default:
>> >>               /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
>> >> --
>> >> 2.9.3
>> >>

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Jakub Kicinski]

On Mon, 23 Jul 2018 11:39:36 +0200, Björn Töpel wrote:
> Den fre 20 juli 2018 kl 22:08 skrev Jakub Kicinski:
> > On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:  
> > > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:  
> > > > rhashtable_lookup() can return NULL. so that NULL pointer
> > > > check routine should be added.
> > > >
> > > > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > > ---
> > > >  net/core/xdp.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > > index 9d1f220..1c12bc7 100644
> > > > --- a/net/core/xdp.c
> > > > +++ b/net/core/xdp.c
> > > > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > > >             rcu_read_lock();
> > > >             /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > > >             xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > > -           xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > +           if (xa)
> > > > +                   xa->zc_alloc->free(xa->zc_alloc, handle);  
> > > hmm...It is not clear to me the "!xa" case don't have to be handled?  
> >
> > Actually I have a more fundamental question about this interface I've
> > been meaning to ask.
> >
> > IIUC free() can happen on any CPU at any time, when whatever device,
> > socket or CPU this got redirected to completed the TX.  IOW there may
> > be multiple producers.  Drivers would need to create spin lock a'la the
> > a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?
> >  
> 
> Jakub, apologies for the slow response. I'm still in
> "holiday/hammock&beer mode", but will be back in a week. :-P

Ah, sorry to interrupt! :)

> The idea with the xdp_return_* functions are that an xdp_buff and
> xdp_frame can have custom allocations schemes. The difference beween
> struct xdp_buff and struct xdp_frame is lifetime. The xdp_buff
> lifetime is within the napi context, whereas xdp_frame can have a
> lifetime longer/outside the napi context. E.g. for a XDP_REDIRECT
> scenario an xdp_buff is converted to a xdp_frame. The conversion is
> done in include/net/xdp.h:convert_to_xdp_frame.
> 
> Currently, the zero-copy MEM_TYPE_ZERO_COPY memtype can *only* be used
> for xdp_buff, meaning that the lifetime is constrained to a napi
> context. Further, given an xdp_buff with memtype MEM_TYPE_ZERO_COPY,
> doing XDP_REDIRECT to a target that is *not* an AF_XDP socket would
> mean converting the xdp_buff to an xdp_frame. The xdp_frame can then
> be free'd on any CPU.
> 
> Note that the xsk_rcv* functions is always called from an napi
> context, and therefore is using the xdp_return_buff calls.
> 
> To answer your question -- no, this fix is *not* needed, because the
> xdp_buff napi constrained, and the xdp_buff will only be free'd on one
> CPU.

Oh, thanks, I missed the check in convert_to_xdp_frame(), so the only
frames which can come back via the free path are out of the error path
in __xsk_rcv_zc()?

That path looks a little surprising too, isn't the expectation that if
xdp_do_redirect() returns an error the driver retains the ownership of
the buffer? 
 
static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
{
	int err = xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, len);

	if (err) {
		xdp_return_buff(xdp);
		xs->rx_dropped++;
	}

	return err;
}

This seems to call xdp_return_buff() *and* return an error.

> > We need some form of internal kernel circulation which would be MPSC.
> > I'm currently hacking up the XSK code to tell me whether the frame was
> > consumed by the correct XSK, and always clone the frame otherwise
> > (claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).
> >
> > I feel like I'm missing something about the code.  Is redirect of
> > ZC/UMEM frame outside the xsk not possible and the only returns we will
> > see are from net/xdp/xsk.c?  That would work, but I don't see such a
> > check.  Help would be appreciated.
> >  
> 
> Right now, this is the case (refer to the TODO in
> convert_to_xdp_frame), i.e. you cannot redirect an ZC/UMEM allocated
> xdp_buff to a target that is not an xsk. This must, obviously, change
> so that an xdp_buff (of MEM_TYPE_ZERO_COPY) can be converted to an
> xdp_frame. The xdp_frame must be able to be free'd from multiple CPUs,
> so here the a more sophisticated allocation scheme is required.
> 
> > Also the fact that XSK bufs can't be freed, only completed, adds to the
> > pain of implementing AF_XDP, we'd certainly need some form of "give
> > back the frame, but I may need it later" SPSC mechanism, otherwise
> > driver writers will have tough time.  Unless, again, I'm missing
> > something about the code :)
> >  
> 
> Yup, moving the recycling scheme from driver to "generic" is a good
> idea! I need to finish up those i40e zerocopy patches first though...

Interesting, FWIW I wasn't necessarily thinking about full recycling,
although that would be the holy grail.  Just a generic way of giving up
buffers for example when user changes ring sizes or brings the device
down.

> (...and I'm very excited that you're doing nfp support for AF_XDP!!!)

Thanks, I'm still way out in the weeds but it's interesting work :)

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Björn Töpel]

Den mån 23 juli 2018 kl 21:58 skrev Jakub Kicinski
<jakub.kicinski@netronome.com>:
>
> On Mon, 23 Jul 2018 11:39:36 +0200, Björn Töpel wrote:
> > Den fre 20 juli 2018 kl 22:08 skrev Jakub Kicinski:
> > > On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:
> > > > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> > > > > rhashtable_lookup() can return NULL. so that NULL pointer
> > > > > check routine should be added.
> > > > >
> > > > > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > > > ---
> > > > >  net/core/xdp.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > > > index 9d1f220..1c12bc7 100644
> > > > > --- a/net/core/xdp.c
> > > > > +++ b/net/core/xdp.c
> > > > > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > > > >             rcu_read_lock();
> > > > >             /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > > > >             xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > > > -           xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > > +           if (xa)
> > > > > +                   xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > hmm...It is not clear to me the "!xa" case don't have to be handled?
> > >
> > > Actually I have a more fundamental question about this interface I've
> > > been meaning to ask.
> > >
> > > IIUC free() can happen on any CPU at any time, when whatever device,
> > > socket or CPU this got redirected to completed the TX.  IOW there may
> > > be multiple producers.  Drivers would need to create spin lock a'la the
> > > a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?
> > >
> >
> > Jakub, apologies for the slow response. I'm still in
> > "holiday/hammock&beer mode", but will be back in a week. :-P
>
> Ah, sorry to interrupt! :)
>

Don't make it a habit! ;-P

> > The idea with the xdp_return_* functions are that an xdp_buff and
> > xdp_frame can have custom allocations schemes. The difference beween
> > struct xdp_buff and struct xdp_frame is lifetime. The xdp_buff
> > lifetime is within the napi context, whereas xdp_frame can have a
> > lifetime longer/outside the napi context. E.g. for a XDP_REDIRECT
> > scenario an xdp_buff is converted to a xdp_frame. The conversion is
> > done in include/net/xdp.h:convert_to_xdp_frame.
> >
> > Currently, the zero-copy MEM_TYPE_ZERO_COPY memtype can *only* be used
> > for xdp_buff, meaning that the lifetime is constrained to a napi
> > context. Further, given an xdp_buff with memtype MEM_TYPE_ZERO_COPY,
> > doing XDP_REDIRECT to a target that is *not* an AF_XDP socket would
> > mean converting the xdp_buff to an xdp_frame. The xdp_frame can then
> > be free'd on any CPU.
> >
> > Note that the xsk_rcv* functions is always called from an napi
> > context, and therefore is using the xdp_return_buff calls.
> >
> > To answer your question -- no, this fix is *not* needed, because the
> > xdp_buff napi constrained, and the xdp_buff will only be free'd on one
> > CPU.
>
> Oh, thanks, I missed the check in convert_to_xdp_frame(), so the only
> frames which can come back via the free path are out of the error path
> in __xsk_rcv_zc()?
>
> That path looks a little surprising too, isn't the expectation that if
> xdp_do_redirect() returns an error the driver retains the ownership of
> the buffer?
>
> static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
> {
>         int err = xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, len);
>
>         if (err) {
>                 xdp_return_buff(xdp);
>                 xs->rx_dropped++;
>         }
>
>         return err;
> }
>
> This seems to call xdp_return_buff() *and* return an error.
>

Ugh, this is indeed an error. The xdp_return buff should be removed.
Thanks for spotting this!

So, yes, the way to get the buffer back (in ZC) to the driver is via
the error path (recycling) or via the UMEM fill queue.

> > > We need some form of internal kernel circulation which would be MPSC.
> > > I'm currently hacking up the XSK code to tell me whether the frame was
> > > consumed by the correct XSK, and always clone the frame otherwise
> > > (claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).
> > >
> > > I feel like I'm missing something about the code.  Is redirect of
> > > ZC/UMEM frame outside the xsk not possible and the only returns we will
> > > see are from net/xdp/xsk.c?  That would work, but I don't see such a
> > > check.  Help would be appreciated.
> > >
> >
> > Right now, this is the case (refer to the TODO in
> > convert_to_xdp_frame), i.e. you cannot redirect an ZC/UMEM allocated
> > xdp_buff to a target that is not an xsk. This must, obviously, change
> > so that an xdp_buff (of MEM_TYPE_ZERO_COPY) can be converted to an
> > xdp_frame. The xdp_frame must be able to be free'd from multiple CPUs,
> > so here the a more sophisticated allocation scheme is required.
> >
> > > Also the fact that XSK bufs can't be freed, only completed, adds to the
> > > pain of implementing AF_XDP, we'd certainly need some form of "give
> > > back the frame, but I may need it later" SPSC mechanism, otherwise
> > > driver writers will have tough time.  Unless, again, I'm missing
> > > something about the code :)
> > >
> >
> > Yup, moving the recycling scheme from driver to "generic" is a good
> > idea! I need to finish up those i40e zerocopy patches first though...
>
> Interesting, FWIW I wasn't necessarily thinking about full recycling,
> although that would be the holy grail.  Just a generic way of giving up
> buffers for example when user changes ring sizes or brings the device
> down.
>
> > (...and I'm very excited that you're doing nfp support for AF_XDP!!!)
>
> Thanks, I'm still way out in the weeds but it's interesting work :)

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Jakub Kicinski]

On Thu, 26 Jul 2018 14:13:20 +0200, Björn Töpel wrote:
> Den mån 23 juli 2018 kl 21:58 skrev Jakub Kicinski:
> > On Mon, 23 Jul 2018 11:39:36 +0200, Björn Töpel wrote:  
> > > Den fre 20 juli 2018 kl 22:08 skrev Jakub Kicinski:  
> > > > On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:  
> > > > > On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:  
> > > > > > rhashtable_lookup() can return NULL. so that NULL pointer
> > > > > > check routine should be added.
> > > > > >
> > > > > > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > > > > > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > > > > > ---
> > > > > >  net/core/xdp.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > > > > index 9d1f220..1c12bc7 100644
> > > > > > --- a/net/core/xdp.c
> > > > > > +++ b/net/core/xdp.c
> > > > > > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > > > > >             rcu_read_lock();
> > > > > >             /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > > > > >             xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > > > > -           xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > > > +           if (xa)
> > > > > > +                   xa->zc_alloc->free(xa->zc_alloc, handle);  
> > > > > hmm...It is not clear to me the "!xa" case don't have to be handled?  
> > > >
> > > > Actually I have a more fundamental question about this interface I've
> > > > been meaning to ask.
> > > >
> > > > IIUC free() can happen on any CPU at any time, when whatever device,
> > > > socket or CPU this got redirected to completed the TX.  IOW there may
> > > > be multiple producers.  Drivers would need to create spin lock a'la the
> > > > a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?
> > > >  
> > >
> > > Jakub, apologies for the slow response. I'm still in
> > > "holiday/hammock&beer mode", but will be back in a week. :-P  
> >
> > Ah, sorry to interrupt! :)
> >  
> 
> Don't make it a habit! ;-P

:-D

> > > The idea with the xdp_return_* functions are that an xdp_buff and
> > > xdp_frame can have custom allocations schemes. The difference beween
> > > struct xdp_buff and struct xdp_frame is lifetime. The xdp_buff
> > > lifetime is within the napi context, whereas xdp_frame can have a
> > > lifetime longer/outside the napi context. E.g. for a XDP_REDIRECT
> > > scenario an xdp_buff is converted to a xdp_frame. The conversion is
> > > done in include/net/xdp.h:convert_to_xdp_frame.
> > >
> > > Currently, the zero-copy MEM_TYPE_ZERO_COPY memtype can *only* be used
> > > for xdp_buff, meaning that the lifetime is constrained to a napi
> > > context. Further, given an xdp_buff with memtype MEM_TYPE_ZERO_COPY,
> > > doing XDP_REDIRECT to a target that is *not* an AF_XDP socket would
> > > mean converting the xdp_buff to an xdp_frame. The xdp_frame can then
> > > be free'd on any CPU.
> > >
> > > Note that the xsk_rcv* functions is always called from an napi
> > > context, and therefore is using the xdp_return_buff calls.
> > >
> > > To answer your question -- no, this fix is *not* needed, because the
> > > xdp_buff napi constrained, and the xdp_buff will only be free'd on one
> > > CPU.  
> >
> > Oh, thanks, I missed the check in convert_to_xdp_frame(), so the only
> > frames which can come back via the free path are out of the error path
> > in __xsk_rcv_zc()?
> >
> > That path looks a little surprising too, isn't the expectation that if
> > xdp_do_redirect() returns an error the driver retains the ownership of
> > the buffer?
> >
> > static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
> > {
> >         int err = xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, len);
> >
> >         if (err) {
> >                 xdp_return_buff(xdp);
> >                 xs->rx_dropped++;
> >         }
> >
> >         return err;
> > }
> >
> > This seems to call xdp_return_buff() *and* return an error.
> >  
> 
> Ugh, this is indeed an error. The xdp_return buff should be removed.
> Thanks for spotting this!
> 
> So, yes, the way to get the buffer back (in ZC) to the driver is via
> the error path (recycling) or via the UMEM fill queue.

Okay, I'm gonna test and submit this later today for bpf tree:

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 72335c2e8108..4e937cd7c17d 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -84,10 +84,8 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 {
        int err = xskq_produce_batch_desc(xs->rx, (u64)xdp->handle, len);
 
-       if (err) {
-               xdp_return_buff(xdp);
+       if (err)
                xs->rx_dropped++;
-       }
 
        return err;
 }

Now the frame return/ZC allocator ->free path is all dead code :S

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Jesper Dangaard Brouer]

On Mon, 23 Jul 2018 11:41:02 +0200
Björn Töpel <bjorn.topel@gmail.com> wrote:

> > >> diff --git a/net/core/xdp.c b/net/core/xdp.c
> > >> index 9d1f220..1c12bc7 100644
> > >> --- a/net/core/xdp.c
> > >> +++ b/net/core/xdp.c
> > >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > >>               rcu_read_lock();
> > >>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
> > >> +             if (xa)
> > >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);  
> > > hmm...It is not clear to me the "!xa" case don't have to be handled?  
> >
> > Thank you for reviewing!
> >
> > Returning NULL pointer is bug case such as calling after use
> > xdp_rxq_info_unreg().
> > so that, I think it can't handle at that moment.
> > we can make __xdp_return to add WARN_ON_ONCE() or
> > add return error code to driver.
> > But I'm not sure if these is useful information.
> >
> > I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
> > because there is no use case of MEM_TYPE_ZERO_COPY yet.
> >  
> 
> Taehee, again, sorry for the slow response and thanks for patch!
> 
> If xa is NULL, the driver has a buggy/broken implementation. What
> would be a proper way of dealing with this? BUG?

Hmm... I don't like these kind of changes to the hot-path code!

You might not realize this, but adding BUG() and WARN_ON() to the code
affect performance in ways you might not realize!  These macros gets
compiled and uses an asm instruction called "ud2".  Seeing the "ud2"
instruction causes the CPUs instruction cache prefetcher to stop.
Thus, if some code ends up below this instruction, this will cause more
i-cache-misses.

I don't know if xa==NULL is even possible, but if it is, then I think
this is a result of a driver mem_reg API usage bug.  And the mem-reg
API is full of WARN's and error messages, exactly to push these kind of
checks out of the fast-path.  There is no need for a BUG() call, as
deref a NULL pointer will case an OOPS, that is easy to read and
understand.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Björn Töpel]

Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <brouer@redhat.com>:
>
> On Mon, 23 Jul 2018 11:41:02 +0200
> Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> > > >> diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > >> index 9d1f220..1c12bc7 100644
> > > >> --- a/net/core/xdp.c
> > > >> +++ b/net/core/xdp.c
> > > >> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > > >>               rcu_read_lock();
> > > >>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > > >>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > > >> -             xa->zc_alloc->free(xa->zc_alloc, handle);
> > > >> +             if (xa)
> > > >> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
> > > > hmm...It is not clear to me the "!xa" case don't have to be handled?
> > >
> > > Thank you for reviewing!
> > >
> > > Returning NULL pointer is bug case such as calling after use
> > > xdp_rxq_info_unreg().
> > > so that, I think it can't handle at that moment.
> > > we can make __xdp_return to add WARN_ON_ONCE() or
> > > add return error code to driver.
> > > But I'm not sure if these is useful information.
> > >
> > > I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
> > > because there is no use case of MEM_TYPE_ZERO_COPY yet.
> > >
> >
> > Taehee, again, sorry for the slow response and thanks for patch!
> >
> > If xa is NULL, the driver has a buggy/broken implementation. What
> > would be a proper way of dealing with this? BUG?
>
> Hmm... I don't like these kind of changes to the hot-path code!
>
> You might not realize this, but adding BUG() and WARN_ON() to the code
> affect performance in ways you might not realize!  These macros gets
> compiled and uses an asm instruction called "ud2".  Seeing the "ud2"
> instruction causes the CPUs instruction cache prefetcher to stop.
> Thus, if some code ends up below this instruction, this will cause more
> i-cache-misses.
>
> I don't know if xa==NULL is even possible, but if it is, then I think
> this is a result of a driver mem_reg API usage bug.  And the mem-reg
> API is full of WARN's and error messages, exactly to push these kind of
> checks out of the fast-path.  There is no need for a BUG() call, as
> deref a NULL pointer will case an OOPS, that is easy to read and
> understand.
>

Jesper, thanks for having a look! So, you're right that if xa==NULL
the driver is "broken/buggy" (as stated earlier!). I agree that
OOPSing on a NULL pointer is as good as a BUG!

The applied patch adds a WARN_ON_ONCE, and I thought best practice was
that a buggy driver shouldn't crash the kernel... What is considered
best practices in these scenarios? *I'd* prefer an OOPS instead of
WARN_ON_ONCE, to catch that buggy driver. Again, that's me. I thought
that most people prefer not crashing, hence the patch. :-)


Björn

> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Daniel Borkmann]

On 08/01/2018 04:43 PM, Björn Töpel wrote:
> Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <brouer@redhat.com>:
>> On Mon, 23 Jul 2018 11:41:02 +0200
>> Björn Töpel <bjorn.topel@gmail.com> wrote:
>>
>>>>>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>>>>>> index 9d1f220..1c12bc7 100644
>>>>>> --- a/net/core/xdp.c
>>>>>> +++ b/net/core/xdp.c
>>>>>> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
>>>>>>               rcu_read_lock();
>>>>>>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
>>>>>>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
>>>>>> -             xa->zc_alloc->free(xa->zc_alloc, handle);
>>>>>> +             if (xa)
>>>>>> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
>>>>> hmm...It is not clear to me the "!xa" case don't have to be handled?
>>>>
>>>> Thank you for reviewing!
>>>>
>>>> Returning NULL pointer is bug case such as calling after use
>>>> xdp_rxq_info_unreg().
>>>> so that, I think it can't handle at that moment.
>>>> we can make __xdp_return to add WARN_ON_ONCE() or
>>>> add return error code to driver.
>>>> But I'm not sure if these is useful information.
>>>>
>>>> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
>>>> because there is no use case of MEM_TYPE_ZERO_COPY yet.
>>>
>>> Taehee, again, sorry for the slow response and thanks for patch!
>>>
>>> If xa is NULL, the driver has a buggy/broken implementation. What
>>> would be a proper way of dealing with this? BUG?
>>
>> Hmm... I don't like these kind of changes to the hot-path code!
>>
>> You might not realize this, but adding BUG() and WARN_ON() to the code
>> affect performance in ways you might not realize!  These macros gets
>> compiled and uses an asm instruction called "ud2".  Seeing the "ud2"
>> instruction causes the CPUs instruction cache prefetcher to stop.
>> Thus, if some code ends up below this instruction, this will cause more
>> i-cache-misses.
>>
>> I don't know if xa==NULL is even possible, but if it is, then I think
>> this is a result of a driver mem_reg API usage bug.  And the mem-reg
>> API is full of WARN's and error messages, exactly to push these kind of
>> checks out of the fast-path.  There is no need for a BUG() call, as
>> deref a NULL pointer will case an OOPS, that is easy to read and
>> understand.
> 
> Jesper, thanks for having a look! So, you're right that if xa==NULL
> the driver is "broken/buggy" (as stated earlier!). I agree that
> OOPSing on a NULL pointer is as good as a BUG!
> 
> The applied patch adds a WARN_ON_ONCE, and I thought best practice was
> that a buggy driver shouldn't crash the kernel... What is considered
> best practices in these scenarios? *I'd* prefer an OOPS instead of
> WARN_ON_ONCE, to catch that buggy driver. Again, that's me. I thought
> that most people prefer not crashing, hence the patch. :-)

In that case, lets send a revert for the patch with a proper analysis
of why it is safe to omit the NULL check which should be placed as a
comment right near the rhashtable_lookup().

Thanks,
Daniel

Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()

[Björn Töpel]

Den ons 1 aug. 2018 kl 22:25 skrev Daniel Borkmann <daniel@iogearbox.net>:
>
> On 08/01/2018 04:43 PM, Björn Töpel wrote:
> > Den ons 1 aug. 2018 kl 16:14 skrev Jesper Dangaard Brouer <brouer@redhat.com>:
> >> On Mon, 23 Jul 2018 11:41:02 +0200
> >> Björn Töpel <bjorn.topel@gmail.com> wrote:
> >>
> >>>>>> diff --git a/net/core/xdp.c b/net/core/xdp.c
> >>>>>> index 9d1f220..1c12bc7 100644
> >>>>>> --- a/net/core/xdp.c
> >>>>>> +++ b/net/core/xdp.c
> >>>>>> @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> >>>>>>               rcu_read_lock();
> >>>>>>               /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> >>>>>>               xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> >>>>>> -             xa->zc_alloc->free(xa->zc_alloc, handle);
> >>>>>> +             if (xa)
> >>>>>> +                     xa->zc_alloc->free(xa->zc_alloc, handle);
> >>>>> hmm...It is not clear to me the "!xa" case don't have to be handled?
> >>>>
> >>>> Thank you for reviewing!
> >>>>
> >>>> Returning NULL pointer is bug case such as calling after use
> >>>> xdp_rxq_info_unreg().
> >>>> so that, I think it can't handle at that moment.
> >>>> we can make __xdp_return to add WARN_ON_ONCE() or
> >>>> add return error code to driver.
> >>>> But I'm not sure if these is useful information.
> >>>>
> >>>> I might have misunderstood scenario of MEM_TYPE_ZERO_COPY
> >>>> because there is no use case of MEM_TYPE_ZERO_COPY yet.
> >>>
> >>> Taehee, again, sorry for the slow response and thanks for patch!
> >>>
> >>> If xa is NULL, the driver has a buggy/broken implementation. What
> >>> would be a proper way of dealing with this? BUG?
> >>
> >> Hmm... I don't like these kind of changes to the hot-path code!
> >>
> >> You might not realize this, but adding BUG() and WARN_ON() to the code
> >> affect performance in ways you might not realize!  These macros gets
> >> compiled and uses an asm instruction called "ud2".  Seeing the "ud2"
> >> instruction causes the CPUs instruction cache prefetcher to stop.
> >> Thus, if some code ends up below this instruction, this will cause more
> >> i-cache-misses.
> >>
> >> I don't know if xa==NULL is even possible, but if it is, then I think
> >> this is a result of a driver mem_reg API usage bug.  And the mem-reg
> >> API is full of WARN's and error messages, exactly to push these kind of
> >> checks out of the fast-path.  There is no need for a BUG() call, as
> >> deref a NULL pointer will case an OOPS, that is easy to read and
> >> understand.
> >
> > Jesper, thanks for having a look! So, you're right that if xa==NULL
> > the driver is "broken/buggy" (as stated earlier!). I agree that
> > OOPSing on a NULL pointer is as good as a BUG!
> >
> > The applied patch adds a WARN_ON_ONCE, and I thought best practice was
> > that a buggy driver shouldn't crash the kernel... What is considered
> > best practices in these scenarios? *I'd* prefer an OOPS instead of
> > WARN_ON_ONCE, to catch that buggy driver. Again, that's me. I thought
> > that most people prefer not crashing, hence the patch. :-)
>
> In that case, lets send a revert for the patch with a proper analysis
> of why it is safe to omit the NULL check which should be placed as a
> comment right near the rhashtable_lookup().
>

I'll do that (as soon as I've double-checked so that I'm not lying)!


Björn

> Thanks,
> Daniel