net: ip, diag -- Add diag interface for raw sockets

In criu we are actively using diag interface to collect sockets present in the system when dumping applications. And while for unix, tcp, udp[lite], packet, netlink it works as expected, the raw sockets do not have. Thus add it.

v2:
- add missing sock_put calls in raw_diag_dump_one (by eric.dumazet@)
- implement @destroy for diag requests (by dsa@)

v3:
- add export of raw_abort for IPv6 (by dsa@)
- pass net-admin flag into inet_sk_diag_fill due to changes in net-next branch (by dsa@)

v4:
- use @pad in struct inet_diag_req_v2 for raw socket protocol specification: raw module carries sockets which may have custom protocol passed from socket() syscall and sole @sdiag_protocol is not enough to match underlied ones
- start reporting protocol specifed in socket() call when sockets are raw ones for the same reason: user space tools like ss may parse this attribute and use it for socket matching

v5 (by eric.dumazet@):
- use sock_hold in raw_sock_get instead of atomic_inc, we're holding (raw_v4_hashinfo|raw_v6_hashinfo)->lock when looking up so counter won't be zero here.

CC David S. Miller <[email protected]>

CC Eric Dumazet <[email protected]>

CC David Ahern <[email protected]>

CC Alexey Kuznetsov <[email protected]>

CC James Morris <[email protected]>

CC Hideaki YOSHIFUJI <[email protected]>

CC Patrick McHardy <[email protected]>

CC Andrey Vagin <[email protected]>

CC Stephen Hemminger <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---

Thanks all for feedback! Take a look please once time permit.

 include/net/raw.h              |    6 +
 include/net/rawv6.h            |    7 +
 include/uapi/linux/inet_diag.h |    5
 net/ipv4/Kconfig               |    8 +
 net/ipv4/Makefile              |    1
 net/ipv4/inet_diag.c           |    9 +
 net/ipv4/raw.c                 |   21 +++
 net/ipv4/raw_diag.c            |  233 +++++++++++++++++++++++++++++++++++++++++
 net/ipv6/raw.c                 |    7 -
 9 files changed, 292 insertions(+), 5 deletions(-)

Index: linux-ml.git/include/net/raw.h
===================================================================

include/net/raw.h +6 -0

@@ -23,6 +23,12 @@
 
 extern struct proto raw_prot;
 
+extern struct raw_hashinfo raw_v4_hashinfo;
+struct sock *__raw_v4_lookup(struct net *net, struct sock *sk,
+			     unsigned short num, __be32 raddr,
+			     __be32 laddr, int dif);
+
+int raw_abort(struct sock *sk, int err);
 void raw_icmp_error(struct sk_buff *, int, u32);
 int raw_local_deliver(struct sk_buff *, int);
 

Index: linux-ml.git/include/net/rawv6.h
===================================================================

include/net/rawv6.h +7 -0

@@ -3,6 +3,13 @@
 
 #include <net/protocol.h>
 
+extern struct raw_hashinfo raw_v6_hashinfo;
+struct sock *__raw_v6_lookup(struct net *net, struct sock *sk,
+			     unsigned short num, const struct in6_addr *loc_addr,
+			     const struct in6_addr *rmt_addr, int dif);
+
+int raw_abort(struct sock *sk, int err);
+
 void raw6_icmp_error(struct sk_buff *, int nexthdr,
 		u8 type, u8 code, int inner_offset, __be32);
 bool raw6_local_deliver(struct sk_buff *, int);

Index: linux-ml.git/include/uapi/linux/inet_diag.h
===================================================================

Quoted 5 lines

include/uapi/linux/inet_diag.h +0 -1

@@ -38,7 +38,10 @@ struct inet_diag_req_v2 {
 	__u8	sdiag_family;
 	__u8	sdiag_protocol;
 	__u8	idiag_ext;
-	__u8	pad;

include/uapi/linux/inet_diag.h struct inet_diag_req_v2 { · line 44

Quoted 4 lines

include/uapi/linux/inet_diag.h +4 -0

+	union {
+		__u8	pad;
+		__u8	sdiag_raw_protocol;	/* SOCK_RAW only, @pad for others */
+	};

Above looks funny. Why is it a union? pad is for exposing a byte-hole for padding/alignment reasons and i doubt anybody is using it.

Should you not just rename it?
Also I notice when things like __raw_v4_lookup() are claiming it is unsigned short instead of a u8?

On Wed, Sep 28, 2016 at 06:08:00AM -0400, Jamal Hadi Salim wrote:
...

Quoted 5 lines

Quoted 5 lines

include/uapi/linux/inet_diag.h +0 -1

@@ -38,7 +38,10 @@ struct inet_diag_req_v2 {
 	__u8	sdiag_family;
 	__u8	sdiag_protocol;
 	__u8	idiag_ext;
-	__u8	pad;

Quoted 8 lines

include/uapi/linux/inet_diag.h struct inet_diag_req_v2 { · line 44

Quoted 4 lines

include/uapi/linux/inet_diag.h +4 -0

+	union {
+		__u8	pad;
+		__u8	sdiag_raw_protocol;	/* SOCK_RAW only, @pad for others */
+	};

Above looks funny. Why is it a union? pad is for exposing a byte-hole for padding/alignment reasons and i doubt anybody is using it.

Someone may have set it to zero explicitly on source level, and the compilation will fail on new kernel then. So no, keeping the name is reasonable.

Quoted 3 lines

Should you not just rename it?
Also I notice when things like __raw_v4_lookup() are claiming it is unsigned short instead of a u8?

The protocol is still up to 255 for a while, is it expected that IPPROTO_MAX will be increased in more-less near future? Of course I can drop the idea of using @pad here and switch to some extended reauest but prefer to stick with simplier solution. Hm?

Quoted 5 lines

Someone may have set it to zero explicitly on source level, and the compilation will fail on new kernel then. So no, keeping the name is reasonable.

I dont know how compilation will fail but you may be right with note:
that is not how pads have been used in the past. They are supposed to cosmetic annotation which indicates "here's a hole; use it in the future if you are looking to add something". And someone in the future can claim them. I am not sure if MBZ philosophy applies.

Quoted 10 lines

Quoted 4 lines

Should you not just rename it?
Also I notice when things like __raw_v4_lookup() are claiming it is unsigned short instead of a u8?

The protocol is still up to 255 for a while, is it expected that IPPROTO_MAX will be increased in more-less near future? Of course I can drop the idea of using @pad here and switch to some extended reauest but prefer to stick with simplier solution. Hm?

Ok. If i understood correctly it was already unsigned short before your patch -so i agree it doesnt matter. Maybe just put a comment to express that if ever protocol goes above 255 it wont be sufficient.

On Wed, Sep 28, 2016 at 06:43:01AM -0400, Jamal Hadi Salim wrote:

Quoted 5 lines

I dont know how compilation will fail but you may be right with note:
that is not how pads have been used in the past. They are supposed to cosmetic annotation which indicates "here's a hole; use it in the future if you are looking to add something". And someone in the future can claim them. I am not sure if MBZ philosophy applies.

This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch.

If protocol goes over u8 then complete inet_diag_req_v2 structure will have to be reworked becaue @sdiag_protocol is u8 as well. IOW, once someone liftup IPPROTO_MAX > 255, he will notice the problem immediately because diag for such module simply stop working properly.

Quoted 6 lines

This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch.

I am conflicted.
A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change.

BTW: There is at least one major structure in inet diag has a hole today and doesnt have a padding indicator.

Quoted 5 lines

If protocol goes over u8 then complete inet_diag_req_v2 structure will have to be reworked becaue @sdiag_protocol is u8 as well. IOW, once someone liftup IPPROTO_MAX > 255, he will notice the problem immediately because diag for such module simply stop working properly.

ok.

cheers,
jamal

Quoted 11 lines · Jamal Hadi Salim

Quoted 6 lines

This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch.

I am conflicted.
A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change.

I think you miss the point what I'm trying to say: currently end-user may have reference to this member (for any reason) and his program will compile and run. If we change the name the compilation procedure fails and this will break API. Yes, referrning @pad is bad idea for userspace code, and yes (!) better to simply rename it but lets do that later, on top, so that if we break something in userspace we could easily revert the oneline change.

I understood well your point;-> Maybe my response was not clear:
_nobody should be fscking fondling pad fields_ setting them or otherwise.

I understood well your point;-> Maybe my response was not clear:
_nobody should be fscking fondling pad fields_ setting them or otherwise.
Maybe let these programs fail. I asked if you knew any such app which did anything with a pad field.

Quoted 11 lines

Quoted 6 lines

This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch.

I am conflicted.
A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change.

I think you miss the point what I'm trying to say: currently end-user may have reference to this member (for any reason) and his program will compile and run. If we change the name the compilation procedure fails and this will break API. Yes, referrning @pad is bad idea for userspace code, and yes (!) better to simply rename it but lets do that later, on top, so that if we break something in userspace we could easily revert the oneline change.

Quoted 10 lines

BTW: There is at least one major structure in inet diag has a hole today and doesnt have a padding indicator.

Quoted 5 lines

If protocol goes over u8 then complete inet_diag_req_v2 structure will have to be reworked becaue @sdiag_protocol is u8 as well. IOW, once someone liftup IPPROTO_MAX > 255, he will notice the problem immediately because diag for such module simply stop working properly.

ok.

Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake.

Quoted 5 lines

Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake.

I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this".

From: Jamal Hadi Salim <[email protected]>
Date: Wed, 28 Sep 2016 08:09:28 -0400

Quoted 7 lines · David Miller

Quoted 5 lines

Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake.

Quoted 2 lines

I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this".

They must initialize it to zero.

Quoted 13 lines · David Miller

From: Jamal Hadi Salim <[email protected]>
Date: Wed, 28 Sep 2016 08:09:28 -0400

Quoted 9 lines · David Miller

Quoted 5 lines

Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake.

I think we need to start labelling any new pad fields added as "Not UAPI. Do not fsck fondle this".

Quoted 2 lines

They must initialize it to zero.

What if in the future actually meant to use 0 for something?;-> For example in Cyrill's case it means PROTO_IP Not sure if it useful to interpret or not but it is part of the enum for protocols.

Quoted 3 lines

They must initialize it to zero.

What if in the future actually meant to use 0 for something?;-> For example in Cyrill's case it means PROTO_IP Not sure if it useful to interpret or not but it is part of the enum for protocols.

Maybe we shouldnt be adding pad fields in these netlink structure definitions then one can liberally add new ones.

Note: inet_diag somewhere has a netlink structure that has a hole. I pointed it out to Eric D. and he said we cant add it now because it would break ABI.

So where do we draw the line for future extensions?

Quoted 3 lines

Note: inet_diag somewhere has a netlink structure that has a hole. I pointed it out to Eric D. and he said we cant add it now because it would break ABI.

Naming holes generated by a compiler for alignment sake should not break abi (because alignments are abi by self), but I may be missing something in context of where the structure you're talking about is present. And what about non-x86 archs? They might have different members alignment requirements.

struct tcp_info.

Sorry - i didnt mean to drag this for long; but the more i think about it, the union is a pragmatic approach.

Quoted 8 lines

I think you miss the point what I'm trying to say: currently end-user may have reference to this member (for any reason) and his program will compile and run. If we change the name the compilation procedure fails and this will break API. Yes, referrning @pad is bad idea for userspace code, and yes (!) better to simply rename it but lets do that later, on top, so that if we break something in userspace we could easily revert the oneline change.

Right, it would be legal for an existing user to have code that explicitly initializes every member of the structure, including 'pad'. So we have to keep that member around, at a minimum, for their sake.

Quoted 8 lines

Quoted 8 lines

BTW: There is at least one major structure in inet diag has a hole today and doesnt have a padding indicator.

Quoted 4 lines

If protocol goes over u8 then complete inet_diag_req_v2 structure will have to be reworked becaue @sdiag_protocol is u8 as well. IOW, once someone liftup IPPROTO_MAX > 255, he will notice the problem immediately because diag for such module simply stop working properly.

Indeed, we need a 16-bit value here.

Quoted 11 lines · Jamal Hadi Salim

Quoted 6 lines

This structure is uapi, so anyone has complete rights to reference @pad in the userspace programs. Sure it would be more clear to remove the @pad completely, but if we choose so I think it's better to do on top instead and then if someone complain we can easily revert the single trivial commit instead of this big patch.

I am conflicted.
A field labelled "pad" does not appear to be valid as "UAPI". It is a cosmetic indicator. If you did sizeof() with or without it being present the value doesnt change.

I think you miss the point what I'm trying to say: currently end-user may have reference to this member (for any reason) and his program will compile and run. If we change the name the compilation procedure fails and this will break API. Yes, referrning @pad is bad idea for userspace code, and yes (!) better to simply rename it but lets do that later, on top, so that if we break something in userspace we could easily revert the oneline change.

Note that some programs could fail to compile with the added union anyway.

Some gcc versions are unable to compile a static init with an union

struct inet_diag_req_v2 foo = { .pad = 0, sdiag_family = AF_INET, };

When I cooked my recent fq commit I simply removed a pad and replaced it :

git show fefa569a9d4bc4 -- include

This is a bit different of course, since struct tc_fq_qd_stats is only one way : Kernel produces the content and gives it to user space.

User space should probably not need to initialize such a structure, but who knows what a programmer can write ;)

Note that some programs could fail to compile with the added union anyway.

Some gcc versions are unable to compile a static init with an union

struct inet_diag_req_v2 foo = { .pad = 0, sdiag_family = AF_INET, };

When I cooked my recent fq commit I simply removed a pad and replaced it :

git show fefa569a9d4bc4 -- include

On Wed, Sep 28, 2016 at 05:57:12AM -0700, Eric Dumazet wrote:
...

Quoted 10 lines

Note that some programs could fail to compile with the added union anyway.

Some gcc versions are unable to compile a static init with an union

struct inet_diag_req_v2 foo = { .pad = 0, sdiag_family = AF_INET, };

When I cooked my recent fq commit I simply removed a pad and replaced it :

Quoted 1 line

git show fefa569a9d4bc4 -- include

Oh, crap :( I've been looking into uapi headers, found that we use anonymous unions (for example include/uapi/linux/bcache.h) and thought it will be safe (and my test builds didn't fail). Are you happen to know which gcc versions cant do that?

Quoted 5 lines

Oh, crap :( I've been looking into uapi headers, found that we use anonymous unions (for example include/uapi/linux/bcache.h) and thought it will be safe (and my test builds didn't fail). Are you happen to know which gcc versions cant do that?

The most recent example I have in mind is a kbuild bot report on the recent TCP BBR patches. We had to rework the patch to avoid the problem.

If I remember well, this was a gcc-4.7, but not on x86.

Quoted 6 lines · Eric Dumazet

Quoted 5 lines

Oh, crap :( I've been looking into uapi headers, found that we use anonymous unions (for example include/uapi/linux/bcache.h) and thought it will be safe (and my test builds didn't fail). Are you happen to know which gcc versions cant do that?

Quoted 4 lines

The most recent example I have in mind is a kbuild bot report on the recent TCP BBR patches. We had to rework the patch to avoid the problem.

If I remember well, this was a gcc-4.7, but not on x86.

4.7 is pretty widespread, so I've to think...

Quoted 2 lines

4.7 is pretty widespread, so I've to think...

Sorry, 4.4.7 it was

https://www.mail-archive.com/[email protected]/msg128714.html