Infiniband stack allows references to freed memory

Infiniband stack allows references to freed memory

[David Miller]

Any time an ipoib_neigh is changed, a sequence like the following is made:

			spin_lock_irqsave(&priv->lock, flags);
			/*
			 * It's safe to call ipoib_put_ah() inside
			 * priv->lock here, because we know that
			 * path->ah will always hold one more reference,
			 * so ipoib_put_ah() will never do more than
			 * decrement the ref count.
			 */
			if (neigh->ah)
				ipoib_put_ah(neigh->ah);
			list_del(&neigh->list);
			ipoib_neigh_free(dev, neigh);
			spin_unlock_irqrestore(&priv->lock, flags);
			ipoib_path_lookup(skb, n, dev);

This doesn't work, because you're leaving a stale pointer to the freed up
ipoib_neigh in the special neigh->ha pointer cookie.  Yes, it even fails
with all the locking done to protect _changes_ to *ipoib_neigh(n), and
with the code in ipoib_neigh_free() that NULLs out the pointer.

The core issue is that read side calls to *to_ipoib_neigh(n) are not
being synchronized at all, they are performed without any locking.  So
whether we hold the lock or not when making changes to *ipoib_neigh(n)
you still can have threads see references to freed up ipoib_neigh
objects.

	cpu 1			cpu 2
	n = *ipoib_neigh()
				*ipoib_neigh() = NULL
				kfree(n)
	n->foo == OOPS

To be honest I wouldn't mind if ipoib managed it's neighbour entries
differently.  The way it works now is getting in the way of some
changes I want to make.  Namely that I want to make it so that
dst_entry objects do not have an attached neighbour, instead
neighbours are always looked up on demand.

Such on-demand neigh lookups require having access to the ipv4/ipv6
header destination address of the packet, and in these ipoib paths we
don't have a real obvious way to get at that without lots of kludgy
tests.

Sticking datastructure pointers into the hardware address of the neigh
is not pretty either :-)

Perhaps the ipoib code can have a private path database it manages
entirely itself, which holds all the necessary information and is
looked up by some generic key which is available easily at transmit
time and does not involve generic neighbour entries.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 12:22 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>
> Any time an ipoib_neigh is changed, a sequence like the following is made:
>
>                        spin_lock_irqsave(&priv->lock, flags);
>                        /*
>                         * It's safe to call ipoib_put_ah() inside
>                         * priv->lock here, because we know that
>                         * path->ah will always hold one more reference,
>                         * so ipoib_put_ah() will never do more than
>                         * decrement the ref count.
>                         */
>                        if (neigh->ah)
>                                ipoib_put_ah(neigh->ah);
>                        list_del(&neigh->list);
>                        ipoib_neigh_free(dev, neigh);
>                        spin_unlock_irqrestore(&priv->lock, flags);
>                        ipoib_path_lookup(skb, n, dev);
>
> This doesn't work, because you're leaving a stale pointer to the freed up
> ipoib_neigh in the special neigh->ha pointer cookie.  Yes, it even fails
> with all the locking done to protect _changes_ to *ipoib_neigh(n), and
> with the code in ipoib_neigh_free() that NULLs out the pointer.
>
> The core issue is that read side calls to *to_ipoib_neigh(n) are not
> being synchronized at all, they are performed without any locking.  So
> whether we hold the lock or not when making changes to *ipoib_neigh(n)
> you still can have threads see references to freed up ipoib_neigh
> objects.
>
>        cpu 1                   cpu 2
>        n = *ipoib_neigh()
>                                *ipoib_neigh() = NULL
>                                kfree(n)
>        n->foo == OOPS
>
> To be honest I wouldn't mind if ipoib managed it's neighbour entries
> differently.  The way it works now is getting in the way of some
> changes I want to make.  Namely that I want to make it so that
> dst_entry objects do not have an attached neighbour, instead
> neighbours are always looked up on demand.
>
> Such on-demand neigh lookups require having access to the ipv4/ipv6
> header destination address of the packet, and in these ipoib paths we
> don't have a real obvious way to get at that without lots of kludgy
> tests.
>
> Sticking datastructure pointers into the hardware address of the neigh
> is not pretty either :-)
>
> Perhaps the ipoib code can have a private path database it manages
> entirely itself, which holds all the necessary information and is
> looked up by some generic key which is available easily at transmit
> time and does not involve generic neighbour entries.

Thanks David.  This is probably the crux of some rare crashes we see
in the ipoib driver around handling the neighbour database.

I've made a few half-hearted stabs at fixing this up but I never really
got past the impedence mismatch between the networking core and
the strange requirements of the IPoIB neighbour handling.

The problem is that we definitely want to use the core networking
stack to resolve neighbours, since we clearly don't want a private
implementation of ARP and IPv6 ND.  But what IPoIB abstractly
calls a "hardware address" is actually an IB GID, which does
uniquely identify a destination but is not actually what we need
to send a packet.

So we do have the whole path database inside the ipoib driver,
and in fact for things like unicast ARPs (which comes without a
neighbour attached) we do lookup the hardware address to an
internal ipoib path.

The problem is that we don't want to do the expensive lookup of
a full 16-byte GID to path for real datapath packets where we do
have a neighbour resolved.  So we try to do the lookup once for
each network stack neighbour.

Perhaps we could do some RCU-ish thing where we don't actually
free the ipoib_neigh immediately?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2012 13:04:58 -0800

> Perhaps we could do some RCU-ish thing where we don't actually
> free the ipoib_neigh immediately?

RCU could be used to fix the race, but the difficulty in converting
this code over to dst_lookup_neigh() would remain and the latter is
more important to me than the former right now :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 1:13 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> Perhaps we could do some RCU-ish thing where we don't actually
>> free the ipoib_neigh immediately?

> RCU could be used to fix the race, but the difficulty in converting
> this code over to dst_lookup_neigh() would remain and the latter is
> more important to me than the former right now :-)

OK.  So do you have any thoughts about what the generic key available
at transmit time (as you alluded to in your first message) might be?

As far as I can figure things, it has to be something roughly
equivalent to the neighbour, since it has to have the same type of
information as what ARP/ND tells us.

I guess I don't understand the on-demand lookup of the neigh.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2012 13:42:30 -0800

> OK.  So do you have any thoughts about what the generic key available
> at transmit time (as you alluded to in your first message) might be?
> 
> As far as I can figure things, it has to be something roughly
> equivalent to the neighbour, since it has to have the same type of
> information as what ARP/ND tells us.
> 
> I guess I don't understand the on-demand lookup of the neigh.

It's "inaddr_any(rt->rt_gateway) ? ip_hdr(skb)->daddr : rt->rt_gateway",
ie. the nexthop.  That's the value we need to do proper on-demand
neigh lookups, it's the "daddr" argument to dst_lookup_neigh().

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Wed, 01 Feb 2012 16:50:41 -0500 (EST)

> From: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Wed, 1 Feb 2012 13:42:30 -0800
> 
>> OK.  So do you have any thoughts about what the generic key available
>> at transmit time (as you alluded to in your first message) might be?
>> 
>> As far as I can figure things, it has to be something roughly
>> equivalent to the neighbour, since it has to have the same type of
>> information as what ARP/ND tells us.
>> 
>> I guess I don't understand the on-demand lookup of the neigh.
> 
> It's "inaddr_any(rt->rt_gateway) ? ip_hdr(skb)->daddr : rt->rt_gateway",
> ie. the nexthop.  That's the value we need to do proper on-demand
> neigh lookups, it's the "daddr" argument to dst_lookup_neigh().

Actually, the argument is "&ip_hdr(skb)->daddr".

So the crux of the matter is making sure we can get at the correct
destination address in the packet header.  Also we might have to
conditionalize the code based upon protocol type, does ipoib handle
ipv6?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 2:02 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> So the crux of the matter is making sure we can get at the correct
> destination address in the packet header.  Also we might have to
> conditionalize the code based upon protocol type, does ipoib handle
> ipv6?

Yes, it does both v4 and v6.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2012 14:06:48 -0800

> On Wed, Feb 1, 2012 at 2:02 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> So the crux of the matter is making sure we can get at the correct
>> destination address in the packet header.  Also we might have to
>> conditionalize the code based upon protocol type, does ipoib handle
>> ipv6?
> 
> Yes, it does both v4 and v6.

So it should be just a matter of checking skb->protocol or similar,
plucking out the daddr from the ipv4/ipv6 header, and then adding
the appropriate neigh release calls.

I have absolutely no way to test this stuff, so could you please
work on the dst_lookup_neigh() conversion of ipoib?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 2:17 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> So it should be just a matter of checking skb->protocol or similar,
> plucking out the daddr from the ipv4/ipv6 header, and then adding
> the appropriate neigh release calls.
>
> I have absolutely no way to test this stuff, so could you please
> work on the dst_lookup_neigh() conversion of ipoib?

I'm happy to do it but I'm still not quite sure I understand what the
end state is.  Is it just a matter of peeking into the skb contents
to get at the daddr, looking up the neigh based on that and then
continuing to handle neighs as we do now?

Does that also work for ARP packets?

Are there any examples in your tree I can crib off of?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Roland Dreier <roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Wed, 1 Feb 2012 15:28:18 -0800

> I'm happy to do it but I'm still not quite sure I understand what the
> end state is.  Is it just a matter of peeking into the skb contents
> to get at the daddr, looking up the neigh based on that and then
> continuing to handle neighs as we do now?

Right, you also will now have a reference to the neigh so you must
release it.

> Does that also work for ARP packets?

ARP packets have no dst_entry, so would need to be handled specially
right now anyways.

> Are there any examples in your tree I can crib off of?

grep for dst_neigh_lookup() in the net-next tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 5:21 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> grep for dst_neigh_lookup() in the net-next tree.

OK, pulled the tree, will take a look.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Or Gerlitz]

On 2/2/2012 3:21 AM, David Miller wrote:
> From: Roland Dreier<roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>
>> I'm happy to do it but I'm still not quite sure I understand what the
>> end state is.  Is it just a matter of peeking into the skb contents
>> to get at the daddr, looking up the neigh based on that and then
>> continuing to handle neighs as we do now?
>
> Right, you also will now have a reference to the neigh so you must release it.
>
>> Does that also work for ARP packets?
>
> ARP packets have no dst_entry, so would need to be handled specially right now anyways.
>
>> Are there any examples in your tree I can crib off of?
>
> grep for dst_neigh_lookup() in the net-next tree.

Hi Dave, Roland,

As was indicated over the thread @ 
http://marc.info/?t=132812805900004&r=1&w=2 the current situation can 
surely lead to races, and I'd be happy to revive it, hopefully towards 
somehow addressing the issue.From my reading through the thread, I 
understand that more or less the following paths (...) were suggested:

#1 convert ipoib to use dst_neigh_lookup instead of dst_get_neighbour_noref
#2 do some RCU-ing where we don't actually free the ipoib_neigh 
immediately, etc

Dave commented that #1 conversion step will allow to merge a change 
where dst_entry objects will not have an attached neighbour any more. 
Alternatively, IPoIB could manage a data structure where ipoib_neighs 
are looked up the packet ipv4/ipv6 header and not from the neighbour.

Performance wise, it seems that the two suggestions should introduce the 
~same overhead, since in the ipv4 case for example, from 
dst_neigh_lookup we would land in __ipv4_neigh_lookup which does a hash 
lookup and we ipoib doesn't use the neighbours any more, some data 
structure lookup (e.g hash) will be used to resolve the ipoib_neigh from 
the destination address.

Dave, as for the conversion to dst_neigh_lookup, looking on net-next, I 
see about 30 hits for dst_get_neighbour_noref vs 13 hits for 
dst_neigh_lookup, so I wasn't sure about your comment re the ipoib 
conversion being what actually remains to make that change happen, can 
you elaborate here a little further?

Also, if ipoib moves to use that api, you made a comment that it will 
have a reference on the neighbour which will need to be released. I took 
a look on the net/atm use case which should to some extent be similar to 
ipoib, I see that neigh_release is called for neighbours retrieved by 
that API, okay.

Just for the sake of example, does the atm code free from the races you 
mention re ipoib? I see that it does stick its own object on the 
neighbour through the priv pointer but wasn't sure if it helps in that 
respect without further RCU-ing things.


Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date: Tue, 17 Apr 2012 11:02:40 +0300

> Dave, as for the conversion to dst_neigh_lookup, looking on net-next,
> I see about 30 hits for dst_get_neighbour_noref vs 13 hits for
> dst_neigh_lookup, so I wasn't sure about your comment re the ipoib
> conversion being what actually remains to make that change happen, can
> you elaborate here a little further?

It is the only major instance that I don't personally have plans for
a dst_neigh_lookup() conversion.

I can't do some of those transformations to dst_neigh_lookup() in
net/ipv4 and net/ipv6 until the others are out of the way.

> Also, if ipoib moves to use that api, you made a comment that it will
> have a reference on the neighbour which will need to be released. I
> took a look on the net/atm use case which should to some extent be
> similar to ipoib, I see that neigh_release is called for neighbours
> retrieved by that API, okay.

Right.

> Just for the sake of example, does the atm code free from the races
> you mention re ipoib? I see that it does stick its own object on the
> neighbour through the priv pointer but wasn't sure if it helps in that
> respect without further RCU-ing things.

I haven't audited ATM for races of any kind, and I have no plans to
do so, sorry.

Please, I did fully audit and analyze the IPoIB case, so please use
one of the approaches suggsted.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Or Gerlitz]

On 4/18/2012 6:04 AM, David Miller wrote:
> It is the only major instance that I don't personally have plans for a 
> dst_neigh_lookup() conversion. I can't do some of those 
> transformations to dst_neigh_lookup() in net/ipv4 and net/ipv6 until 
> the others are out of the way. 

understood, thanks for clarifying this out.

>> Just for the sake of example, does the atm code free from the races you mention re ipoib? I see that it does stick its own object on the  neighbour through the priv pointer but wasn't sure if it helps in that respect without further RCU-ing things.
>
> I haven't audited ATM for races of any kind, and I have no plans to  do so, sorry. Please, I did fully audit and analyze the IPoIB case, so please use one of the approaches suggsted.

okay, so please let me phrase that differently, would it be correct to 
say that if we stick to the 1st approach of still somehow associating 
the ipoib_neigh with the neighbour - no matter through what means we do 
that - RCU-ing is must here. Specifically, the neighbour->priv 
mechanism  wouldn't get us a way from adding that protection? or maybe 
it could make our life easier if we turn to use it?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[David Miller]

From: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date: Wed, 18 Apr 2012 08:58:16 +0300

> okay, so please let me phrase that differently, would it be correct to
> say that if we stick to the 1st approach of still somehow associating
> the ipoib_neigh with the neighbour - no matter through what means we
> do that - RCU-ing is must here. Specifically, the neighbour->priv
> mechanism wouldn't get us a way from adding that protection? or maybe
> it could make our life easier if we turn to use it?

I would need to have to go back to the original thread and reread and
reabsorb the full analysis I did back then to answer this, and I
really don't have time to do that right now.

You're going to have to figure out how to fix this race properly on
your own or with someone else's help.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Or Gerlitz]

On 4/18/2012 9:15 AM, David Miller wrote:
> I would need to have to go back to the original thread and reread and 
> reabsorb the full analysis I did back then to answer this, and I 
> really don't have time to do that right now. You're going to have to 
> figure out how to fix this race properly on your own or with someone 
> else's help.

Sure, I didn't ask you to redo the analysis for the ipoib use case, its 
well understood.

I just wanted to see if you can spare few sentences regarding the 
neighbour priv mechanism you added in the below sequence, all are your 
patches from July 2011 which I think means they were introduced in 3.1.0

Specifically, is there an existing RCU neighbour related framework/call 
that a driver which uses neighbour_priv(n) and probably also the 
neigh_construct/destroy ndo callbacks, can use to make sure that 
modifying the private area isn't racy with readers of that area?

BTW I see that in commit 596b9b68ef118f7409afbc78487263e08ef96261 you 
changed IPoIB to set
dev->neigh_priv_len to be sizeof(ipoib_neigh), so you've started a 
possible porting here..

Or.

I took a look on the below series and also commit 
32092ecf0644e91070f9eff4f6e1edda8f90aecc
"atm: clip: Use device neigh support on top of "arp_tbl" " which made 
atm to use the neigh_construct call.


> commit da6a8fa0275e2178c44a875374cae80d057538d1
>
>     neigh: Add device constructor/destructor capability.
>
>     If the neigh entry has device private state, it will need
>     constructor/destructor ops.
>
> commit 869759b9e4160fb8a8d25bc3b4ce3b658523aebb
>
>     atm: clip: Convert over to neighbour_priv()
>
>
> commit 76cc714ed5fe6ed90aad5c52ff3030f1f4e22a48
>
>     neigh: Do not set tbl->entry_size in ipv4/ipv6 neigh tables.
>
>     Let the core self-size the neigh entry based upon the key length.
>
>
> commit 596b9b68ef118f7409afbc78487263e08ef96261
>
>     neigh: Add infrastructure for allocating device neigh privates.
>
>     netdev->neigh_priv_len records the private area length.
>
>     This will trigger for neigh_table objects which set tbl->entry_size
>     to zero, and the first instances of this will be forthcoming.
>
>
> commit 5b8b0060cbd6332ae5d1fa0bec0e8e211248d0e7
>
>     neigh: Get rid of neigh_table->kmem_cachep
>
>     We are going to alloc for device specific private areas for
>     neighbour entries, and in order to do that we have to move
>     away from the fixed allocation size enforced by using
>     neigh_table->kmem_cachep
>
>     As a nice side effect we can now use kfree_rcu().
>
>     Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
>
> commit 1026fec8739663621d64216ba939c23bc1d089b7
>
>     neigh: Create mechanism for generic neigh private areas.
>
>     The implementation private sits right after the primary_key memory.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Jason Gunthorpe]

On Wed, Feb 01, 2012 at 01:42:30PM -0800, Roland Dreier wrote:

> > RCU could be used to fix the race, but the difficulty in converting
> > this code over to dst_lookup_neigh() would remain and the latter is
> > more important to me than the former right now :-)
> 
> OK.  So do you have any thoughts about what the generic key available
> at transmit time (as you alluded to in your first message) might be?

I thought a big reason the IPoIB stuff was setup this way was to fit
in with older kernels (eg we cannot change the neigh structures)?

Would making the allocation of the neigh structure returned by
dst_lookup_neigh() large enough to hold the ARP/ND HW addr and also
the IB path record solve these problems? At least for UD?

Haven't looked at the dst_lookup_neigh work from David though, so I
could be way off :)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 2:26 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> I thought a big reason the IPoIB stuff was setup this way was to fit
> in with older kernels (eg we cannot change the neigh structures)?

No, IPoIB tries to be as "native" as possible.  The problem as I see
it is that IPoIB does standard ARP/ND, but then it has an extra step
(path record lookup) to turn that neighbour into an address handle we
can use to actually send a packet.  And the networking stack doesn't
really have a good place to add that extra step -- we can override the
neighbour discovery / ARP layer, but we can't tack on another step.

> Would making the allocation of the neigh structure returned by
> dst_lookup_neigh() large enough to hold the ARP/ND HW addr and also
> the IB path record solve these problems? At least for UD?

I don't think so, because we don't always have a neighbour when
we get a packet to send (eg unicast ARP), and we need a place to
queue up packets while we do the path lookup (the ipoib_neigh.queue
list does that now).  So we definitely need paths/AHs separate from
neighs for the first part and we need more than just data in the neigh
for the second part.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Jason Gunthorpe]

On Wed, Feb 01, 2012 at 03:32:26PM -0800, Roland Dreier wrote:

> > Would making the allocation of the neigh structure returned by
> > dst_lookup_neigh() large enough to hold the ARP/ND HW addr and also
> > the IB path record solve these problems? At least for UD?
> 
> I don't think so, because we don't always have a neighbour when
> we get a packet to send (eg unicast ARP), and we need a place to
> queue up packets while we do the path lookup (the ipoib_neigh.queue
> list does that now).  So we definitely need paths/AHs separate from
> neighs for the first part and we need more than just data in the neigh
> for the second part.

I guess I'm missing something. I thought the stack didn't have
a unicast hwaddr separate from a neighbour structure? Isn't that where
it is stored? Unicast arp does have a destination IP address and a
destination hwaddr...

To elaborate a bit more what was idly in my head, if the new approach
is to always lookup the neighbor on demand, and that can include an
ARP to the network, then can't we piggyback on that same process to
issue a path record? With a 1:1 correspondence between unicast
neighbour, path record, and AH they can all be stored in the same
allocation, with the same lifetime. Thus you don't need the second
part, and maybe the first part becomes an inefficient special case?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Infiniband stack allows references to freed memory

[Roland Dreier]

On Wed, Feb 1, 2012 at 5:02 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> I guess I'm missing something. I thought the stack didn't have
> a unicast hwaddr separate from a neighbour structure? Isn't that where
> it is stored? Unicast arp does have a destination IP address and a
> destination hwaddr...

Look at unicast_arp_send() and related code in ipoib_main.c...
Basically we have to handle the case where we have no neighbour
struct when we're sending a unicast ARP.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html