Re: [ofa-general] [PATCH] IPoIB: check multicast address format (V2)
thanks for updating, applied. ___ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format (V2)
Check that the format of the multicast link address is correct before
taking it from dev->mc_list to priv->multicast_list. This way we never
try to send a bogus address to the SA, and prevents badness from
erronous 'ip maddr addr add', broken bonding drivers, or whatever.
Signed-off-by: Jason Gunthorpe
---
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 19 +++
1 files changed, 19 insertions(+), 0 deletions(-)
> The idea seems sound but checkpatch.pl gives 6 errors for this small
> patch! Also:
Indeed, sorry, I don't do this very often, forgot that step. I added
the 6 missing spaces.
> name of the function could make it clearer what the expected return
> value is ... eg mcast_addr_is_valid() or something like that.
Yes, done
> > + if (addrlen != 20)
> We have INFINIBAND_ALEN defined, seems better than a magic # here.
Great, I looked for that for a few mins..
> > + if (memcmp(addr,broadcast,6) != 0)
> Personal taste here, but "if (foo != 0)" always seems silly to me when
> we could just do "if (foo)" -- haven't looked at what usage of memcmp()
> is more idiomatic in the kernel tho.
OK, much of the rest of the kernel is without the operator. I do it
this way because the discordance of:
if (cmp(a, b)) means a is not b,
if (!cmp(a, b)) means a is b
if (is_something(a)) means a is something
hurts my brain, even though I know that is how it all
works..
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 425e311..a6485c4 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -758,6 +758,20 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
}
}
+static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
+const u8 *broadcast)
+{
+ if (addrlen != INFINIBAND_ALEN)
+ return 0;
+ /* reserved QPN, prefix, scope */
+ if (memcmp(addr, broadcast, 6))
+ return 0;
+ /* signature lower, pkey */
+ if (memcmp(addr + 7, broadcast + 7, 3))
+ return 0;
+ return 1;
+}
+
void ipoib_mcast_restart_task(struct work_struct *work)
{
struct ipoib_dev_priv *priv =
@@ -791,6 +805,11 @@ void ipoib_mcast_restart_task(struct work_struct *work)
for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
union ib_gid mgid;
+ if (!ipoib_mcast_addr_is_valid(mclist->dmi_addr,
+ mclist->dmi_addrlen,
+ dev->broadcast))
+ continue;
+
memcpy(mgid.raw, mclist->dmi_addr + 4, sizeof mgid);
mcast = __ipoib_mcast_find(dev, &mgid);
--
1.5.4.2
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
The idea seems sound but checkpatch.pl gives 6 errors for this small patch! Also: > +static int check_mcast(const u8 *addr,unsigned int addrlen, > + const u8 *broadcast) name of the function could make it clearer what the expected return value is ... eg mcast_addr_is_valid() or something like that. > +if (addrlen != 20) We have INFINIBAND_ALEN defined, seems better than a magic # here. > +if (memcmp(addr,broadcast,6) != 0) Personal taste here, but "if (foo != 0)" always seems silly to me when we could just do "if (foo)" -- haven't looked at what usage of memcmp() is more idiomatic in the kernel tho. - R. ___ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
Moni Shoua wrote: Jason Gunthorpe wrote: Is this true? That is pretty ugly, but probably manageable.. Unfortunately, losing routes is a side effect of closing the device Moni, I tend to agree with Jason's about this being OTOH ugly but OTOH manageable, maybe you can send a patch to the kernel bonding document that states to re-set non trivial routes for ipoib bonds after their initial establishment (will save you some support cases...) Or. ___ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
Jason Gunthorpe wrote:
> On Wed, Aug 26, 2009 at 12:07:51PM +0300, Or Gerlitz wrote:
>
>> isn't Jason's approach enough for the bonding case?! I saw that your
>> patch ("bonding: clean muticast addresses when device changes type"
>
> I think working versions of all three patches are required:
> 1) Fix the bonding driver. Otherwise the right groups might not be
> joined.
> 2) Check the address format, to protect against 'ip maddr add' and
> other wakkyness
> 3) Fix the timeout handling, so mlid exhaustion and other SA side
> errors are handled elegantly.
>
> All are bugs..
>
>> and maybe also in mainline .31-rcX . However, it has the
>> down-side-effect of e.g loosing routes already set for the the bond
>> while adding the underline IPoIB devices, so if Jason's patch is
>> enough
>
> Is this true? That is pretty ugly, but probably manageable..
>
Yes it's true but I'm not sure it's ugly. Changing device type is not a common
event and requires device ops change which I think is better to do when the
device is closed. Unfortunately, losing routes is a side effect of closing the
device but it might be necessary.
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
On Wed, Aug 26, 2009 at 12:07:51PM +0300, Or Gerlitz wrote:
> isn't Jason's approach enough for the bonding case?! I saw that your
> patch ("bonding: clean muticast addresses when device changes type"
I think working versions of all three patches are required:
1) Fix the bonding driver. Otherwise the right groups might not be
joined.
2) Check the address format, to protect against 'ip maddr add' and
other wakkyness
3) Fix the timeout handling, so mlid exhaustion and other SA side
errors are handled elegantly.
All are bugs..
> and maybe also in mainline .31-rcX . However, it has the
> down-side-effect of e.g loosing routes already set for the the bond
> while adding the underline IPoIB devices, so if Jason's patch is
> enough
Is this true? That is pretty ugly, but probably manageable..
--
Jason Gunthorpe (780)4406067x832
Chief Technology Officer, Obsidian Research Corp Edmonton, Canada
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
Jason Gunthorpe wrote:
Check that the format of the multicast link address is correct before taking it from
dev->mc_list to priv->multicast_list. This way we never try to send a bogus
address to the SA, and prevents badness from erronous 'ip maddr addr add', broken
bonding drivers, or whatever.
Jason,
This is great (and simple!) idea, lets go for it.
Same problem Moni was working on, but lets just address it directly. There is
work to try and fix the bonding driver but no fixed version is in mainline yet.
This is a cheap and simple work around that is worth having even once the
driver is fixed.
Moni,
isn't Jason's approach enough for the bonding case?! I saw that your
patch ("bonding: clean muticast addresses when device changes type"
commit e36b9d16c6a6d0f59803b3ef04ff3c22c3844c10) is present in net-next
and maybe also in mainline .31-rcX . However, it has the
down-side-effect of e.g loosing routes already set for the the bond
while adding the underline IPoIB devices, so if Jason's patch is enough
we can just ask to revert the bonding fix saying we have something better.
Or.
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
> > Why not check validity with something that looks like the reverse operation > of the kernel function that maps ip -> link mcast addresses? Sorry. Please ignore this comment. Jason's patch does exactly that. ___ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
Jason Gunthorpe wrote:
> Check that the format of the multicast link address is correct before
> taking it from dev->mc_list to priv->multicast_list. This way we never
> try to send a bogus address to the SA, and prevents badness from
> erronous 'ip maddr addr add', broken bonding drivers, or whatever.
>
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 18 ++
> 1 files changed, 18 insertions(+), 0 deletions(-)
>
> Same problem Moni was working on, but lets just address it directly.
>
> There is work to try and fix the bonding driver but no fixed version
> is in mainline yet. This is a cheap and simple work around that is
> worth having even once the driver is fixed.
>
> Despite this, I think it is still necessary to do something like Moni
> was trying - to prevent the MCG join queue from head of line blocking
> on a single bad SA response. This can happen even if everything is
> correct.
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 425e311..973a24b 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -758,6 +758,20 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
> }
> }
>
> +static int check_mcast(const u8 *addr,unsigned int addrlen,
> +const u8 *broadcast)
> +{
> + if (addrlen != 20)
> + return 0;
> + /* QPN, scope, reserved, sigature upper */
> + if (memcmp(addr,broadcast,6) != 0)
> + return 0;
> + /* signature lower, pkey */
> + if (memcmp(addr + 7,broadcast+7,3) != 0)
> + return 0;
> + return 1;
> +}
> +
> void ipoib_mcast_restart_task(struct work_struct *work)
> {
> struct ipoib_dev_priv *priv =
> @@ -791,6 +805,10 @@ void ipoib_mcast_restart_task(struct work_struct *work)
> for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
> union ib_gid mgid;
>
> + if (!check_mcast(mclist->dmi_addr,mclist->dmi_addrlen,
> + dev->broadcast))
> + continue;
> +
> memcpy(mgid.raw, mclist->dmi_addr + 4, sizeof mgid);
>
> mcast = __ipoib_mcast_find(dev, &mgid);
Why not check validity with something that looks like the reverse operation
of the kernel function that maps ip -> link mcast addresses?
for example this is the function for IPv4
static inline void ip_ib_mc_map(__be32 naddr, const unsigned char *broadcast,
char *buf)
{
__u32 addr;
unsigned char scope = broadcast[5] & 0xF;
buf[0] = 0;/* Reserved */
buf[1] = 0xff; /* Multicast QPN */
buf[2] = 0xff;
buf[3] = 0xff;
addr= ntohl(naddr);
buf[4] = 0xff;
buf[5] = 0x10 | scope; /* scope from broadcast address */
buf[6] = 0x40; /* IPv4 signature */
buf[7] = 0x1b;
buf[8] = broadcast[8]; /* P_Key */
buf[9] = broadcast[9];
buf[10] = 0;
buf[11] = 0;
buf[12] = 0;
buf[13] = 0;
buf[14] = 0;
buf[15] = 0;
buf[19] = addr & 0xff;
addr >>= 8;
buf[18] = addr & 0xff;
addr >>= 8;
buf[17] = addr & 0xff;
addr >>= 8;
buf[16] = addr & 0x0f;
}
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [ofa-general] [PATCH] IPoIB: check multicast address format
Jason Gunthorpe wrote: > Check that the format of the multicast link address is correct before > taking it from dev->mc_list to priv->multicast_list. This way we never > try to send a bogus address to the SA, and prevents badness from > erronous 'ip maddr addr add', broken bonding drivers, or whatever. > > Signed-off-by: Jason Gunthorpe > --- > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 18 ++ > 1 files changed, 18 insertions(+), 0 deletions(-) > > Same problem Moni was working on, but lets just address it directly. > > There is work to try and fix the bonding driver but no fixed version > is in mainline yet. This is a cheap and simple work around that is > worth having even once the driver is fixed. > The fix is available from at least 2.6.31-rc2. However, I still need to check your claim that it doesn't work for you. > Despite this, I think it is still necessary to do something like Moni > was trying - to prevent the MCG join queue from head of line blocking > on a single bad SA response. This can happen even if everything is > correct. > I'll resend my patch since it has been a long time from the last time I sent it. ___ general mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
[ofa-general] [PATCH] IPoIB: check multicast address format
Check that the format of the multicast link address is correct before
taking it from dev->mc_list to priv->multicast_list. This way we never
try to send a bogus address to the SA, and prevents badness from
erronous 'ip maddr addr add', broken bonding drivers, or whatever.
Signed-off-by: Jason Gunthorpe
---
drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 18 ++
1 files changed, 18 insertions(+), 0 deletions(-)
Same problem Moni was working on, but lets just address it directly.
There is work to try and fix the bonding driver but no fixed version
is in mainline yet. This is a cheap and simple work around that is
worth having even once the driver is fixed.
Despite this, I think it is still necessary to do something like Moni
was trying - to prevent the MCG join queue from head of line blocking
on a single bad SA response. This can happen even if everything is
correct.
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 425e311..973a24b 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -758,6 +758,20 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
}
}
+static int check_mcast(const u8 *addr,unsigned int addrlen,
+ const u8 *broadcast)
+{
+ if (addrlen != 20)
+ return 0;
+ /* QPN, scope, reserved, sigature upper */
+ if (memcmp(addr,broadcast,6) != 0)
+ return 0;
+ /* signature lower, pkey */
+ if (memcmp(addr + 7,broadcast+7,3) != 0)
+ return 0;
+ return 1;
+}
+
void ipoib_mcast_restart_task(struct work_struct *work)
{
struct ipoib_dev_priv *priv =
@@ -791,6 +805,10 @@ void ipoib_mcast_restart_task(struct work_struct *work)
for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
union ib_gid mgid;
+ if (!check_mcast(mclist->dmi_addr,mclist->dmi_addrlen,
+dev->broadcast))
+ continue;
+
memcpy(mgid.raw, mclist->dmi_addr + 4, sizeof mgid);
mcast = __ipoib_mcast_find(dev, &mgid);
--
1.5.4.2
___
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
