[PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...);

[PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...);

[Norman Maurer]

From: Norman Maurer <norman_maurer@apple.com>

Support for UDP_GRO was added in the past but the implementation for
getsockopt was missed which did lead to an error when we tried to
retrieve the setting for UDP_GRO. This patch adds the missing switch
case for UDP_GRO

Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
Signed-off-by: Norman Maurer <norman_maurer@apple.com>
---
 net/ipv4/udp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 4a0478b17243..99d743eb9dc4 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2754,6 +2754,10 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
 		val = up->gso_size;
 		break;
 
+	case UDP_GRO:
+		val = up->gro_enabled;
+		break;
+
 	/* The following two cannot be changed on UDP sockets, the return is
 	 * always 0 (which corresponds to the full checksum coverage of UDP). */
 	case UDPLITE_SEND_CSCOV:
-- 
2.30.2

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Paolo Abeni]

Hello,

On Thu, 2021-03-25 at 20:56 +0100, Norman Maurer wrote:
> From: Norman Maurer <norman_maurer@apple.com>
> 
> Support for UDP_GRO was added in the past but the implementation for
> getsockopt was missed which did lead to an error when we tried to
> retrieve the setting for UDP_GRO. This patch adds the missing switch
> case for UDP_GRO
> 
> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
> Signed-off-by: Norman Maurer <norman_maurer@apple.com>

The patch LGTM, but please cc the blamed commit author in when you add
a 'Fixes' tag (me in this case ;)

Also please specify a target tree, either 'net' or 'net-next', in the
patch subj. Being declared as a fix, this should target 'net'.

One thing you can do to simplifies the maintainer's life, would be post
a v2 with the correct tag (and ev. obsolete this patch in patchwork).

Side note: I personally think this is more a new feature (is adds
getsockopt support for UDP_GRO) than a fix, so I would not have added
the 'Fixes' tag and I would have targeted net-next, but it's just my
opinion.

Cheers,

Paolo

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Norman Maurer]

Hi,

> On 26. Mar 2021, at 10:36, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Hello,
> 
> On Thu, 2021-03-25 at 20:56 +0100, Norman Maurer wrote:
>> From: Norman Maurer <norman_maurer@apple.com>
>> 
>> Support for UDP_GRO was added in the past but the implementation for
>> getsockopt was missed which did lead to an error when we tried to
>> retrieve the setting for UDP_GRO. This patch adds the missing switch
>> case for UDP_GRO
>> 
>> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
>> Signed-off-by: Norman Maurer <norman_maurer@apple.com>
> 
> The patch LGTM, but please cc the blamed commit author in when you add
> a 'Fixes' tag (me in this case ;)

Noted for the next time… 

> 
> Also please specify a target tree, either 'net' or 'net-next', in the
> patch subj. Being declared as a fix, this should target 'net'.
> 

Ok noted

> One thing you can do to simplifies the maintainer's life, would be post
> a v2 with the correct tag (and ev. obsolete this patch in patchwork).

I am quite new to contribute patches to the kernel so I am not sure how I would “obsolete” this patch and make a v2. If you can give me some pointers I am happy to do so.


> 
> Side note: I personally think this is more a new feature (is adds
> getsockopt support for UDP_GRO) than a fix, so I would not have added
> the 'Fixes' tag and I would have targeted net-next, but it's just my
> opinion.

I see… For me it seemed more like a bug as I can’t think of a reason why only setsockopt should be supported for an option but not getsockopt. But it may be just my opinion :)

> 
> Cheers,
> 
> Paolo
> 

Thanks
Norman

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Paolo Abeni]

On Fri, 2021-03-26 at 11:22 +0100, Norman Maurer wrote:
> On 26. Mar 2021, at 10:36, Paolo Abeni <pabeni@redhat.com> wrote:
> > One thing you can do to simplifies the maintainer's life, would be post
> > a v2 with the correct tag (and ev. obsolete this patch in patchwork).
> 
> I am quite new to contribute patches to the kernel so I am not sure
> how I would “obsolete” this patch and make a v2. If you can give me
> some pointers I am happy to do so.

Well, I actually gave you a bad advice about fiddling with patchwork.

The autoritative documentation:

Documentation/networking/netdev-FAQ.rst

(inside the kernel tree) suggests to avoid it.

Just posting a v2 will suffice.

Thanks!

Paolo

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Norman Maurer]

Friendly ping… 

As this missing change was most likely an oversight in the original commit I do think it should go into 5.12 and subsequently stable as well. That’s also the reason why I didn’t send a v2 and changed the commit message / subject for the patch. For me it clearly is a bug and not a new feature.


Thanks
Norman


> On 26. Mar 2021, at 13:22, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Fri, 2021-03-26 at 11:22 +0100, Norman Maurer wrote:
>> On 26. Mar 2021, at 10:36, Paolo Abeni <pabeni@redhat.com> wrote:
>>> One thing you can do to simplifies the maintainer's life, would be post
>>> a v2 with the correct tag (and ev. obsolete this patch in patchwork).
>> 
>> I am quite new to contribute patches to the kernel so I am not sure
>> how I would “obsolete” this patch and make a v2. If you can give me
>> some pointers I am happy to do so.
> 
> Well, I actually gave you a bad advice about fiddling with patchwork.
> 
> The autoritative documentation:
> 
> Documentation/networking/netdev-FAQ.rst
> 
> (inside the kernel tree) suggests to avoid it.
> 
> Just posting a v2 will suffice.
> 
> Thanks!
> 
> Paolo
> 

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Paolo Abeni]

On Wed, 2021-03-31 at 15:10 +0200, Norman Maurer wrote:
> As this missing change was most likely an oversight in the original
> commit I do think it should go into 5.12 and subsequently stable as
> well. That’s also the reason why I didn’t send a v2 and changed the
> commit message / subject for the patch. For me it clearly is a bug
> and not a new feature.

I have no strong opinion against that (sorry, I hoped that was clear in
my reply).

Please go ahead.

Thanks,

Paolo

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[David Ahern]

On 3/31/21 7:10 AM, Norman Maurer wrote:
> Friendly ping… 
> 
> As this missing change was most likely an oversight in the original commit I do think it should go into 5.12 and subsequently stable as well. That’s also the reason why I didn’t send a v2 and changed the commit message / subject for the patch. For me it clearly is a bug and not a new feature.
> 
> 

I agree that it should be added to net

If you do not see it here:
  https://patchwork.kernel.org/project/netdevbpf/list/

you need to re-send and clearly mark it as [PATCH net]. Make sure it has
a Fixes tag.

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Norman Maurer]

> On 1. Apr 2021, at 02:18, David Ahern <dsahern@gmail.com> wrote:
> 
> On 3/31/21 7:10 AM, Norman Maurer wrote:
>> Friendly ping… 
>> 
>> As this missing change was most likely an oversight in the original commit I do think it should go into 5.12 and subsequently stable as well. That’s also the reason why I didn’t send a v2 and changed the commit message / subject for the patch. For me it clearly is a bug and not a new feature.
>> 
>> 
> 
> I agree that it should be added to net
> 
> If you do not see it here:
>  https://patchwork.kernel.org/project/netdevbpf/list/
> 
> you need to re-send and clearly mark it as [PATCH net]. Make sure it has
> a Fixes tag.
> 

Done: https://lore.kernel.org/netdev/20210401065917.78025-1-norman_maurer@apple.com/

Thanks a lot of the review and help.

Bye
Norman

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Norman Maurer]

Hi,

> On 26. Mar 2021, at 10:36, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Hello,
> 
> On Thu, 2021-03-25 at 20:56 +0100, Norman Maurer wrote:
>> From: Norman Maurer <norman_maurer@apple.com>
>> 
>> Support for UDP_GRO was added in the past but the implementation for
>> getsockopt was missed which did lead to an error when we tried to
>> retrieve the setting for UDP_GRO. This patch adds the missing switch
>> case for UDP_GRO
>> 
>> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
>> Signed-off-by: Norman Maurer <norman_maurer@apple.com>
> 
> The patch LGTM, but please cc the blamed commit author in when you add
> a 'Fixes' tag (me in this case ;)

Noted for the next time… 

> 
> Also please specify a target tree, either 'net' or 'net-next', in the
> patch subj. Being declared as a fix, this should target 'net'.
> 

Ok noted

> One thing you can do to simplifies the maintainer's life, would be post
> a v2 with the correct tag (and ev. obsolete this patch in patchwork).

I am quite new to contribute patches to the kernel so I am not sure how I would “obsolete” this patch and make a v2. If you can give me some pointers I am happy to do so.


> 
> Side note: I personally think this is more a new feature (is adds
> getsockopt support for UDP_GRO) than a fix, so I would not have added
> the 'Fixes' tag and I would have targeted net-next, but it's just my
> opinion.

I see… For me it seemed more like a bug as I can’t think of a reason why only setsockopt should be supported for an option but not getsockopt. But it may be just my opinion :)

> 
> Cheers,
> 
> Paolo
> 

Thanks
Norman

Re: [PATCH] udp: Add support for getsockopt(..., ..., UDP_GRO, ..., ...)

[Norman Maurer]

Hi,

> On 26. Mar 2021, at 10:36, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Hello,
> 
> On Thu, 2021-03-25 at 20:56 +0100, Norman Maurer wrote:
>> From: Norman Maurer <norman_maurer@apple.com>
>> 
>> Support for UDP_GRO was added in the past but the implementation for
>> getsockopt was missed which did lead to an error when we tried to
>> retrieve the setting for UDP_GRO. This patch adds the missing switch
>> case for UDP_GRO
>> 
>> Fixes: e20cf8d3f1f7 ("udp: implement GRO for plain UDP sockets.")
>> Signed-off-by: Norman Maurer <norman_maurer@apple.com>
> 
> The patch LGTM, but please cc the blamed commit author in when you add
> a 'Fixes' tag (me in this case ;)

Noted for the next time… 

> 
> Also please specify a target tree, either 'net' or 'net-next', in the
> patch subj. Being declared as a fix, this should target 'net'.
> 

Ok noted

> One thing you can do to simplifies the maintainer's life, would be post
> a v2 with the correct tag (and ev. obsolete this patch in patchwork).

I am quite new to contribute patches to the kernel so I am not sure how I would “obsolete” this patch and make a v2. If you can give me some pointers I am happy to do so.


> 
> Side note: I personally think this is more a new feature (is adds
> getsockopt support for UDP_GRO) than a fix, so I would not have added
> the 'Fixes' tag and I would have targeted net-next, but it's just my
> opinion.

I see… For me it seemed more like a bug as I can’t think of a reason why only setsockopt should be supported for an option but not getsockopt. But it may be just my opinion :)

> 
> Cheers,
> 
> Paolo
> 

Thanks
Norman