Submitting fixes for bugs: Re: tun device patch for IPv6

Submitting fixes for bugs: Re: tun device patch for IPv6

[Natalie Protasevich]

On Mon, Jun 2, 2008 at 6:53 AM, Zabele, Stephen (US SSA)
<steve.zabele@baesystems.com> wrote:
> Natalie,
>
>>Steve,
>>Can you please submit the patch to LKML and to Maxim
> (maxk@qualcomm.com) >via email (if you haven't already)?
>
> The patch is accessible from the bottom of the bugzilla web page, but
> I'm also providing it again here for reference. I' not sure how to
> submit it to LKML other than through the bugzilla mechanism. Can you
> provide me a pointer??
>
> Thanks!
>
> Steve
>

Hi Steve,
This issue is pretty generic. Bug reporters often produce fixes, that
remain in bugzilla not being harvested, simply because bugzilla is not
designed for submitting patches. Even developers who work on bugzilla
and leave their patches attached to the bug don't get those
incorporated until they submit it conventional way.

To submit the patch properly follow
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt. There are
other pointers like http://linux.yyz.us/patch-format.html,
http://lxr.linux.no/linux/Documentation/SubmittingPatches. You can
look at any submitted patch as a sample and examine the format; note
that added/changed/removed lines produced with diffstat.

Maintainers deal with great number of patches, so in order to be able
to use your patch they have go receive it in proper format i.e. with
conventional comments and headers, such that it complies with source
keeping process (not mentioning that it should apply to latest tree,
so they don't have to port it...)
Any patch that fixes a problem is valuable, so by submitting it proper
way you'll make sure (1) your patch will get the best review possible
(2)  you will get proper credit for it. Then you post it to LKML and
CC to subsystem maintainer and/or developers heavily involved with
this module.
Thanks,
--Natalie

> --- Cut here ---
>
> --- a/drivers/net/tun.c 2008-02-11 01:06:32.000000000 -0500
> +++ b/drivers/net/tun.c 2008-03-18 15:30:55.000000000 -0400
> @@ -243,8 +243,26 @@
>
>                if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
>                        return -EFAULT;
> -       }
> -
> +
> +        } else if ((tun->flags & TUN_TYPE_MASK) == TUN_TUN_DEV) {
> +                if (iv->iov_len) {
> +                        unsigned char verbuf;
> +
> +                        if (copy_from_user((void *)&verbuf,
> iv->iov_base, sizeof(verbuf)))
> +                                return -EFAULT;
> +
> +                        switch (verbuf & 0xf0) {
> +                          case 0x40:
> +                            break;
> +                          case 0x60:
> +                            pi.proto = htons(ETH_P_IPV6);
> +                            break;
> +                          default:
> +                            return -EINVAL;
> +                        }
> +                }
> +        }
> +
>        if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
>                align = NET_IP_ALIGN;
>
>

Re: Submitting fixes for bugs: Re: tun device patch for IPv6

[Max Krasnyansky]

btw I naked similar patch a couple of times already.
TUN driver provide PI (protocol info) extension that lets uses set packet
type. Please use that instead.

Max

Natalie Protasevich wrote:
> On Mon, Jun 2, 2008 at 6:53 AM, Zabele, Stephen (US SSA)
> <steve.zabele@baesystems.com> wrote:
>> Natalie,
>>
>>> Steve,
>>> Can you please submit the patch to LKML and to Maxim
>> (maxk@qualcomm.com) >via email (if you haven't already)?
>>
>> The patch is accessible from the bottom of the bugzilla web page, but
>> I'm also providing it again here for reference. I' not sure how to
>> submit it to LKML other than through the bugzilla mechanism. Can you
>> provide me a pointer??
>>
>> Thanks!
>>
>> Steve
>>
> 
> Hi Steve,
> This issue is pretty generic. Bug reporters often produce fixes, that
> remain in bugzilla not being harvested, simply because bugzilla is not
> designed for submitting patches. Even developers who work on bugzilla
> and leave their patches attached to the bug don't get those
> incorporated until they submit it conventional way.
> 
> To submit the patch properly follow
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt. There are
> other pointers like http://linux.yyz.us/patch-format.html,
> http://lxr.linux.no/linux/Documentation/SubmittingPatches. You can
> look at any submitted patch as a sample and examine the format; note
> that added/changed/removed lines produced with diffstat.
> 
> Maintainers deal with great number of patches, so in order to be able
> to use your patch they have go receive it in proper format i.e. with
> conventional comments and headers, such that it complies with source
> keeping process (not mentioning that it should apply to latest tree,
> so they don't have to port it...)
> Any patch that fixes a problem is valuable, so by submitting it proper
> way you'll make sure (1) your patch will get the best review possible
> (2)  you will get proper credit for it. Then you post it to LKML and
> CC to subsystem maintainer and/or developers heavily involved with
> this module.
> Thanks,
> --Natalie
> 
>> --- Cut here ---
>>
>> --- a/drivers/net/tun.c 2008-02-11 01:06:32.000000000 -0500
>> +++ b/drivers/net/tun.c 2008-03-18 15:30:55.000000000 -0400
>> @@ -243,8 +243,26 @@
>>
>>                if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
>>                        return -EFAULT;
>> -       }
>> -
>> +
>> +        } else if ((tun->flags & TUN_TYPE_MASK) == TUN_TUN_DEV) {
>> +                if (iv->iov_len) {
>> +                        unsigned char verbuf;
>> +
>> +                        if (copy_from_user((void *)&verbuf,
>> iv->iov_base, sizeof(verbuf)))
>> +                                return -EFAULT;
>> +
>> +                        switch (verbuf & 0xf0) {
>> +                          case 0x40:
>> +                            break;
>> +                          case 0x60:
>> +                            pi.proto = htons(ETH_P_IPV6);
>> +                            break;
>> +                          default:
>> +                            return -EINVAL;
>> +                        }
>> +                }
>> +        }
>> +
>>        if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
>>                align = NET_IP_ALIGN;
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

RE: Submitting fixes for bugs: Re: tun device patch for IPv6

[Zabele, Stephen (US SSA)]

Max,

I am confused by your comment. As you point out, the TUN driver does
have an option that allows users to provide the protocol information as
an extended header. The problem is that there is a lot of existing IPv4
code that does not use this approach, and we'd like to make it easy for
folsk to port their existing code to work with IPv6.

Hence, the patch provided below allows existing software that does not
use the extra protocol information to work with IPv6.

If you are suggesting that "people shouldn't be using the non-PI option"
for the TUN device, then the option should be taken out to force the
issue rather than leaving an incomplete implementations lying around the
kernel and making folks spend a lot of hours trying to debug why the
non-PI version doesn't work with IPv6 -- like I did. 

I disagree with this assertion, however, because providing this
information forces a lot of extra (and in my opinion unnecessary) coding
in user space.

Steve

-----Original Message-----
From: Max Krasnyansky [mailto:maxk@qualcomm.com] 
Sent: Monday, June 02, 2008 11:36 PM
To: Natalie Protasevich
Cc: Zabele, Stephen (US SSA); Andrew Morton; LKML
Subject: Re: Submitting fixes for bugs: Re: tun device patch for IPv6

btw I naked similar patch a couple of times already.
TUN driver provide PI (protocol info) extension that lets uses set
packet
type. Please use that instead.

Max

Natalie Protasevich wrote:
> On Mon, Jun 2, 2008 at 6:53 AM, Zabele, Stephen (US SSA)
> <steve.zabele@baesystems.com> wrote:
>> Natalie,
>>
>>> Steve,
>>> Can you please submit the patch to LKML and to Maxim
>> (maxk@qualcomm.com) >via email (if you haven't already)?
>>
>> The patch is accessible from the bottom of the bugzilla web page, but
>> I'm also providing it again here for reference. I' not sure how to
>> submit it to LKML other than through the bugzilla mechanism. Can you
>> provide me a pointer??
>>
>> Thanks!
>>
>> Steve
>>
> 
> Hi Steve,
> This issue is pretty generic. Bug reporters often produce fixes, that
> remain in bugzilla not being harvested, simply because bugzilla is not
> designed for submitting patches. Even developers who work on bugzilla
> and leave their patches attached to the bug don't get those
> incorporated until they submit it conventional way.
> 
> To submit the patch properly follow
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt. There are
> other pointers like http://linux.yyz.us/patch-format.html,
> http://lxr.linux.no/linux/Documentation/SubmittingPatches. You can
> look at any submitted patch as a sample and examine the format; note
> that added/changed/removed lines produced with diffstat.
> 
> Maintainers deal with great number of patches, so in order to be able
> to use your patch they have go receive it in proper format i.e. with
> conventional comments and headers, such that it complies with source
> keeping process (not mentioning that it should apply to latest tree,
> so they don't have to port it...)
> Any patch that fixes a problem is valuable, so by submitting it proper
> way you'll make sure (1) your patch will get the best review possible
> (2)  you will get proper credit for it. Then you post it to LKML and
> CC to subsystem maintainer and/or developers heavily involved with
> this module.
> Thanks,
> --Natalie
> 
>> --- Cut here ---
>>
>> --- a/drivers/net/tun.c 2008-02-11 01:06:32.000000000 -0500
>> +++ b/drivers/net/tun.c 2008-03-18 15:30:55.000000000 -0400
>> @@ -243,8 +243,26 @@
>>
>>                if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
>>                        return -EFAULT;
>> -       }
>> -
>> +
>> +        } else if ((tun->flags & TUN_TYPE_MASK) == TUN_TUN_DEV) {
>> +                if (iv->iov_len) {
>> +                        unsigned char verbuf;
>> +
>> +                        if (copy_from_user((void *)&verbuf,
>> iv->iov_base, sizeof(verbuf)))
>> +                                return -EFAULT;
>> +
>> +                        switch (verbuf & 0xf0) {
>> +                          case 0x40:
>> +                            break;
>> +                          case 0x60:
>> +                            pi.proto = htons(ETH_P_IPV6);
>> +                            break;
>> +                          default:
>> +                            return -EINVAL;
>> +                        }
>> +                }
>> +        }
>> +
>>        if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
>>                align = NET_IP_ALIGN;
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: Submitting fixes for bugs: Re: tun device patch for IPv6

[Max Krasnyanskiy]

Zabele, Stephen (US SSA) wrote:
> Max,
> 
> I am confused by your comment. As you point out, the TUN driver does
> have an option that allows users to provide the protocol information as
> an extended header. The problem is that there is a lot of existing IPv4
> code that does not use this approach, and we'd like to make it easy for
> folsk to port their existing code to work with IPv6.
> 
> Hence, the patch provided below allows existing software that does not
> use the extra protocol information to work with IPv6.
> 
> If you are suggesting that "people shouldn't be using the non-PI option"
> for the TUN device, then the option should be taken out to force the
> issue rather than leaving an incomplete implementations lying around the
> kernel and making folks spend a lot of hours trying to debug why the
> non-PI version doesn't work with IPv6 -- like I did. 
> 
> I disagree with this assertion, however, because providing this
> information forces a lot of extra (and in my opinion unnecessary) coding
> in user space.

I guess the argument makes sense. And it's been submitted a few times already. 
So if people insist :). The patch is not the best way to do it though. I do 
not think we need an extra copy_from_user(). I'll either dig out an original 
patch or cook up a new one and then push it upstream along a couple of other 
TUN bug fixes.

Max




> 
> Steve
> 
> -----Original Message-----
> From: Max Krasnyansky [mailto:maxk@qualcomm.com] 
> Sent: Monday, June 02, 2008 11:36 PM
> To: Natalie Protasevich
> Cc: Zabele, Stephen (US SSA); Andrew Morton; LKML
> Subject: Re: Submitting fixes for bugs: Re: tun device patch for IPv6
> 
> btw I naked similar patch a couple of times already.
> TUN driver provide PI (protocol info) extension that lets uses set
> packet
> type. Please use that instead.
> 
> Max
> 
> Natalie Protasevich wrote:
>> On Mon, Jun 2, 2008 at 6:53 AM, Zabele, Stephen (US SSA)
>> <steve.zabele@baesystems.com> wrote:
>>> Natalie,
>>>
>>>> Steve,
>>>> Can you please submit the patch to LKML and to Maxim
>>> (maxk@qualcomm.com) >via email (if you haven't already)?
>>>
>>> The patch is accessible from the bottom of the bugzilla web page, but
>>> I'm also providing it again here for reference. I' not sure how to
>>> submit it to LKML other than through the bugzilla mechanism. Can you
>>> provide me a pointer??
>>>
>>> Thanks!
>>>
>>> Steve
>>>
>> Hi Steve,
>> This issue is pretty generic. Bug reporters often produce fixes, that
>> remain in bugzilla not being harvested, simply because bugzilla is not
>> designed for submitting patches. Even developers who work on bugzilla
>> and leave their patches attached to the bug don't get those
>> incorporated until they submit it conventional way.
>>
>> To submit the patch properly follow
>> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt. There are
>> other pointers like http://linux.yyz.us/patch-format.html,
>> http://lxr.linux.no/linux/Documentation/SubmittingPatches. You can
>> look at any submitted patch as a sample and examine the format; note
>> that added/changed/removed lines produced with diffstat.
>>
>> Maintainers deal with great number of patches, so in order to be able
>> to use your patch they have go receive it in proper format i.e. with
>> conventional comments and headers, such that it complies with source
>> keeping process (not mentioning that it should apply to latest tree,
>> so they don't have to port it...)
>> Any patch that fixes a problem is valuable, so by submitting it proper
>> way you'll make sure (1) your patch will get the best review possible
>> (2)  you will get proper credit for it. Then you post it to LKML and
>> CC to subsystem maintainer and/or developers heavily involved with
>> this module.
>> Thanks,
>> --Natalie
>>
>>> --- Cut here ---
>>>
>>> --- a/drivers/net/tun.c 2008-02-11 01:06:32.000000000 -0500
>>> +++ b/drivers/net/tun.c 2008-03-18 15:30:55.000000000 -0400
>>> @@ -243,8 +243,26 @@
>>>
>>>                if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
>>>                        return -EFAULT;
>>> -       }
>>> -
>>> +
>>> +        } else if ((tun->flags & TUN_TYPE_MASK) == TUN_TUN_DEV) {
>>> +                if (iv->iov_len) {
>>> +                        unsigned char verbuf;
>>> +
>>> +                        if (copy_from_user((void *)&verbuf,
>>> iv->iov_base, sizeof(verbuf)))
>>> +                                return -EFAULT;
>>> +
>>> +                        switch (verbuf & 0xf0) {
>>> +                          case 0x40:
>>> +                            break;
>>> +                          case 0x60:
>>> +                            pi.proto = htons(ETH_P_IPV6);
>>> +                            break;
>>> +                          default:
>>> +                            return -EINVAL;
>>> +                        }
>>> +                }
>>> +        }
>>> +
>>>        if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
>>>                align = NET_IP_ALIGN;
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 
> 

RE: Submitting fixes for bugs: Re: tun device patch for IPv6

[Zabele, Stephen (US SSA)]

Thanks Max, much appreciated.

>From what I understand, the copy_from_user is needed to protect kernels
that are running on multiprocessor hosts -- on some other work we were
doing a while back, we had left the copy out and were just using the
provided pointer directly to access information in the buffer -- and it
was causing intermittent kernel panics.

Steve

-----Original Message-----
From: Max Krasnyanskiy [mailto:maxk@qualcomm.com] 
Sent: Tuesday, June 03, 2008 2:56 PM
To: Zabele, Stephen (US SSA)
Cc: Natalie Protasevich; Andrew Morton; LKML
Subject: Re: Submitting fixes for bugs: Re: tun device patch for IPv6

Zabele, Stephen (US SSA) wrote:
> Max,
> 
> I am confused by your comment. As you point out, the TUN driver does
> have an option that allows users to provide the protocol information
as
> an extended header. The problem is that there is a lot of existing
IPv4
> code that does not use this approach, and we'd like to make it easy
for
> folsk to port their existing code to work with IPv6.
> 
> Hence, the patch provided below allows existing software that does not
> use the extra protocol information to work with IPv6.
> 
> If you are suggesting that "people shouldn't be using the non-PI
option"
> for the TUN device, then the option should be taken out to force the
> issue rather than leaving an incomplete implementations lying around
the
> kernel and making folks spend a lot of hours trying to debug why the
> non-PI version doesn't work with IPv6 -- like I did. 
> 
> I disagree with this assertion, however, because providing this
> information forces a lot of extra (and in my opinion unnecessary)
coding
> in user space.

I guess the argument makes sense. And it's been submitted a few times
already. 
So if people insist :). The patch is not the best way to do it though. I
do 
not think we need an extra copy_from_user(). I'll either dig out an
original 
patch or cook up a new one and then push it upstream along a couple of
other 
TUN bug fixes.

Max




> 
> Steve
> 
> -----Original Message-----
> From: Max Krasnyansky [mailto:maxk@qualcomm.com] 
> Sent: Monday, June 02, 2008 11:36 PM
> To: Natalie Protasevich
> Cc: Zabele, Stephen (US SSA); Andrew Morton; LKML
> Subject: Re: Submitting fixes for bugs: Re: tun device patch for IPv6
> 
> btw I naked similar patch a couple of times already.
> TUN driver provide PI (protocol info) extension that lets uses set
> packet
> type. Please use that instead.
> 
> Max
> 
> Natalie Protasevich wrote:
>> On Mon, Jun 2, 2008 at 6:53 AM, Zabele, Stephen (US SSA)
>> <steve.zabele@baesystems.com> wrote:
>>> Natalie,
>>>
>>>> Steve,
>>>> Can you please submit the patch to LKML and to Maxim
>>> (maxk@qualcomm.com) >via email (if you haven't already)?
>>>
>>> The patch is accessible from the bottom of the bugzilla web page,
but
>>> I'm also providing it again here for reference. I' not sure how to
>>> submit it to LKML other than through the bugzilla mechanism. Can you
>>> provide me a pointer??
>>>
>>> Thanks!
>>>
>>> Steve
>>>
>> Hi Steve,
>> This issue is pretty generic. Bug reporters often produce fixes, that
>> remain in bugzilla not being harvested, simply because bugzilla is
not
>> designed for submitting patches. Even developers who work on bugzilla
>> and leave their patches attached to the bug don't get those
>> incorporated until they submit it conventional way.
>>
>> To submit the patch properly follow
>> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt. There are
>> other pointers like http://linux.yyz.us/patch-format.html,
>> http://lxr.linux.no/linux/Documentation/SubmittingPatches. You can
>> look at any submitted patch as a sample and examine the format; note
>> that added/changed/removed lines produced with diffstat.
>>
>> Maintainers deal with great number of patches, so in order to be able
>> to use your patch they have go receive it in proper format i.e. with
>> conventional comments and headers, such that it complies with source
>> keeping process (not mentioning that it should apply to latest tree,
>> so they don't have to port it...)
>> Any patch that fixes a problem is valuable, so by submitting it
proper
>> way you'll make sure (1) your patch will get the best review possible
>> (2)  you will get proper credit for it. Then you post it to LKML and
>> CC to subsystem maintainer and/or developers heavily involved with
>> this module.
>> Thanks,
>> --Natalie
>>
>>> --- Cut here ---
>>>
>>> --- a/drivers/net/tun.c 2008-02-11 01:06:32.000000000 -0500
>>> +++ b/drivers/net/tun.c 2008-03-18 15:30:55.000000000 -0400
>>> @@ -243,8 +243,26 @@
>>>
>>>                if(memcpy_fromiovec((void *)&pi, iv, sizeof(pi)))
>>>                        return -EFAULT;
>>> -       }
>>> -
>>> +
>>> +        } else if ((tun->flags & TUN_TYPE_MASK) == TUN_TUN_DEV) {
>>> +                if (iv->iov_len) {
>>> +                        unsigned char verbuf;
>>> +
>>> +                        if (copy_from_user((void *)&verbuf,
>>> iv->iov_base, sizeof(verbuf)))
>>> +                                return -EFAULT;
>>> +
>>> +                        switch (verbuf & 0xf0) {
>>> +                          case 0x40:
>>> +                            break;
>>> +                          case 0x60:
>>> +                            pi.proto = htons(ETH_P_IPV6);
>>> +                            break;
>>> +                          default:
>>> +                            return -EINVAL;
>>> +                        }
>>> +                }
>>> +        }
>>> +
>>>        if ((tun->flags & TUN_TYPE_MASK) == TUN_TAP_DEV)
>>>                align = NET_IP_ALIGN;
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 
>