[PATCH] ipoib: fix hard_header return value

[PATCH] ipoib: fix hard_header return value

[Doug Ledford]

If you have a patched up dhcp server (and dhclient), they will use
AF_PACKET/SOCK_DGRAM pair to send dhcp packets over IPoIB.  This has
worked since forever if you use OFED kernels or one of the distribution
kernels.  However, when testing an upstream kernel, it has been broken
for a very long time (I tested 2.6.34, 2.6.38, 3.0, 3.1, 3.8, HEAD).

It turns out that the hard_header routine in ipoib is not following
the API and is returning 0 even when it pushed data onto the skb.  This
then causes af_packet.c to overwrite the header just pushed with data
from user space.  This header is immediately referenced in the
ipoib_start_xmit routine, so I'm wondering how this ever worked in
distro/ofed kernels that also have this bug, but fixing the bug here
makes things work in upstream kernels.

Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 8534afd..31dd2a7 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -828,7 +828,7 @@ static int ipoib_hard_header(struct sk_buff *skb,
 	 */
 	memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);
 
-	return 0;
+	return sizeof *header;
 }
 
 static void ipoib_set_mcast_list(struct net_device *dev)
-- 
1.8.1.2

--
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: [PATCH] ipoib: fix hard_header return value

[Bart Van Assche]

On 03/26/13 17:24, Doug Ledford wrote:
> If you have a patched up dhcp server (and dhclient), they will use
> AF_PACKET/SOCK_DGRAM pair to send dhcp packets over IPoIB.  This has
> worked since forever if you use OFED kernels or one of the distribution
> kernels.  However, when testing an upstream kernel, it has been broken
> for a very long time (I tested 2.6.34, 2.6.38, 3.0, 3.1, 3.8, HEAD).
>
> It turns out that the hard_header routine in ipoib is not following
> the API and is returning 0 even when it pushed data onto the skb.  This
> then causes af_packet.c to overwrite the header just pushed with data
> from user space.  This header is immediately referenced in the
> ipoib_start_xmit routine, so I'm wondering how this ever worked in
> distro/ofed kernels that also have this bug, but fixing the bug here
> makes things work in upstream kernels.
>
> Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 8534afd..31dd2a7 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -828,7 +828,7 @@ static int ipoib_hard_header(struct sk_buff *skb,
>   	 */
>   	memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);
>
> -	return 0;
> +	return sizeof *header;
>   }
>
>   static void ipoib_set_mcast_list(struct net_device *dev)

Checkpatch recommends since some time to use sizeof(e) instead of sizeof 
e, isn't it ?

Bart.
--
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: [PATCH] ipoib: fix hard_header return value

[Roland Dreier]

> Checkpatch recommends since some time to use sizeof(e) instead of sizeof e,
> isn't it ?

I actually prefer "sizeof e" since sizeof is an operator, not a
function.  "sizeof(e)" looks just as silly as "return(e)" to me.

I'll apply this patch soon, it's a good catch.
--
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: [PATCH] ipoib: fix hard_header return value

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On 03/26/2013 12:46 PM, Roland Dreier wrote:
>> Checkpatch recommends since some time to use sizeof(e) instead of sizeof e,
>> isn't it ?
> 
> I actually prefer "sizeof e" since sizeof is an operator, not a
> function.  "sizeof(e)" looks just as silly as "return(e)" to me.
> 
> I'll apply this patch soon, it's a good catch.
> 

Cool, thanks!

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH] ipoib: fix hard_header return value

[Jason Gunthorpe]

On Tue, Mar 26, 2013 at 09:46:28AM -0700, Roland Dreier wrote:
> > Checkpatch recommends since some time to use sizeof(e) instead of sizeof e,
> > isn't it ?
> 
> I actually prefer "sizeof e" since sizeof is an operator, not a
> function.  "sizeof(e)" looks just as silly as "return(e)" to me.

Sizeof is used as an expression, return is not.

They have different precedence rules:
 return e + 1; // == return (e + 1)
vs
 sizeof e + 1;  // == sizeof(e) + 1

Or weirder:
 return (void *)x; // OK
vs
 sizeof (void *)x; // <-- compile error

Coding sizeof as a function call frees the reader from having to
check/know the obscure precedence rules for sizeof.

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: [PATCH] ipoib: fix hard_header return value

[Doug Ledford]

On 03/26/2013 02:16 PM, Jason Gunthorpe wrote:
> On Tue, Mar 26, 2013 at 09:46:28AM -0700, Roland Dreier wrote:
>>> Checkpatch recommends since some time to use sizeof(e) instead of sizeof e,
>>> isn't it ?
>>
>> I actually prefer "sizeof e" since sizeof is an operator, not a
>> function.  "sizeof(e)" looks just as silly as "return(e)" to me.
> 
> Sizeof is used as an expression, return is not.
> 
> They have different precedence rules:
>  return e + 1; // == return (e + 1)
> vs
>  sizeof e + 1;  // == sizeof(e) + 1
> 
> Or weirder:
>  return (void *)x; // OK
> vs
>  sizeof (void *)x; // <-- compile error
> 
> Coding sizeof as a function call frees the reader from having to
> check/know the obscure precedence rules for sizeof.

Personally, I couldn't care less.  But about 5 lines outside of the
context of this patch in the same function is another use of that same
expression.  The real reason I used the form I did was to match the
style of that other instance.  I prepared a separate patch to convert
both of them to the checkpatch preferred style until I saw Roland say he
didn't want it.  But what I wouldn't have done is submitted a patch that
caused a single function to have two different styles within 8 lines of
each other just because some nanny state nag script tells me too.

--
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: [PATCH] ipoib: fix hard_header return value

[Or Gerlitz]

Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> If you have a patched up dhcp server (and dhclient),

Could you be more specific, I assume you refer to the ISC dhcp bits,
which version and which patches? AFAIK they don't give you access to
their source repo but rather only to drops plus possibly patches which
is a bit more tedious to follow, oh wel...

> they will use AF_PACKET/SOCK_DGRAM pair to send dhcp packets over IPoIB.


> This has worked since forever if you use OFED kernels or one of the distribution
> kernels.  However, when testing an upstream kernel, it has been broken
> for a very long time (I tested 2.6.34, 2.6.38, 3.0, 3.1, 3.8, HEAD).

IMO doesn't seem relevant to the upstream commit message

> It turns out that the hard_header routine in ipoib is not following
> the API and is returning 0 even when it pushed data onto the skb.  This
> then causes af_packet.c to overwrite the header just pushed with data
> from user space.  This header is immediately referenced in the
> ipoib_start_xmit routine

cool, I assume we want this fix to go for -stable after spending some
time upstream, e.g probably by the time 3.9 is released and some more
testing is done on the patch (I'll advocate for that @ MLNX, copied
some folks now) get that to -stable too.

Erez, in the code we use internally which is based on upstream 3.7 do
we have DHCP/IPoIB working without this patch?

> so I'm wondering how this ever worked in
> distro/ofed kernels that also have this bug, but fixing the bug here
> makes things work in upstream kernels.

same for the last three lines


Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/
ipoib_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 8534afd..31dd2a7 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -828,7 +828,7 @@ static int ipoib_hard_header(struct sk_buff *skb,
         */
        memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);

-       return 0;
+       return sizeof *header;
 }

 static void ipoib_set_mcast_list(struct net_device *dev)
--
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: [PATCH] ipoib: fix hard_header return value

[Doug Ledford]

On 04/01/13 17:25, Or Gerlitz wrote:
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> If you have a patched up dhcp server (and dhclient),
> 
> Could you be more specific, I assume you refer to the ISC dhcp bits,
> which version and which patches?

Any version of dhcp server, and the improved-xid and lpf-ib patches
primarily.  We've carried those patches forever, but as far as I know,
they still have not been taken by ISC.  Without them, dhcp server will
only work with a cooked socket interface.  You can't use raw as the
socket type when compiling or else it won't work on IB.  With the
patches, you can enable the raw socket method, and on IB it will fall
back to use PACKET instead.

> AFAIK they don't give you access to
> their source repo but rather only to drops plus possibly patches which
> is a bit more tedious to follow, oh wel...
> 
>> they will use AF_PACKET/SOCK_DGRAM pair to send dhcp packets over IPoIB.
> 
> 
>> This has worked since forever if you use OFED kernels or one of the distribution
>> kernels.  However, when testing an upstream kernel, it has been broken
>> for a very long time (I tested 2.6.34, 2.6.38, 3.0, 3.1, 3.8, HEAD).
> 
> IMO doesn't seem relevant to the upstream commit message

I disagree.  I don't buy the whole "we are upstream, nothing else
matters or is relevant" philosophy.  The truth of the matter is that
there is essentially a fork between upstream and OFED.  I plan on
spending some time bringing some of the relevant fixes present in OFED
and not upstream back to upstream.  In the context of attempting to
manually merge some of that fork back together, I see no reason
whatsoever to ignore relevant historical information during that process.

>> It turns out that the hard_header routine in ipoib is not following
>> the API and is returning 0 even when it pushed data onto the skb.  This
>> then causes af_packet.c to overwrite the header just pushed with data
>> from user space.  This header is immediately referenced in the
>> ipoib_start_xmit routine
> 
> cool, I assume we want this fix to go for -stable after spending some
> time upstream, e.g probably by the time 3.9 is released and some more
> testing is done on the patch (I'll advocate for that @ MLNX, copied
> some folks now) get that to -stable too.

Yes, it can go to -stable.  But, given that no one has noticed before
now that it doesn't work, I'm guessing not many people are using
straight upstream (which is something that needs fixed IMO).

> Erez, in the code we use internally which is based on upstream 3.7 do
> we have DHCP/IPoIB working without this patch?
> 
>> so I'm wondering how this ever worked in
>> distro/ofed kernels that also have this bug, but fixing the bug here
>> makes things work in upstream kernels.
> 
> same for the last three lines
> 
> 
> Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/ulp/ipoib/
> ipoib_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> index 8534afd..31dd2a7 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> @@ -828,7 +828,7 @@ static int ipoib_hard_header(struct sk_buff *skb,
>          */
>         memcpy(cb->hwaddr, daddr, INFINIBAND_ALEN);
> 
> -       return 0;
> +       return sizeof *header;
>  }
> 
>  static void ipoib_set_mcast_list(struct net_device *dev)
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford

Infiniband specific RPMs available at
	      http://people.redhat.com/dledford/Infiniband
--
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