Re: [PATCH v5] net: ip, diag -- Add diag interface for raw sockets

From: Jamal Hadi Salim
Date: Wed Sep 28 2016 - 06:08:16 EST

On 16-09-28 05:03 AM, Cyrill Gorcunov wrote:
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.

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

- 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@)

- 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 <davem@xxxxxxxxxxxxx>
CC: Eric Dumazet <eric.dumazet@xxxxxxxxx>
CC: David Ahern <dsa@xxxxxxxxxxxxxxxxxxx>
CC: Alexey Kuznetsov <kuznet@xxxxxxxxxxxxx>
CC: James Morris <jmorris@xxxxxxxxx>
CC: Hideaki YOSHIFUJI <yoshfuji@xxxxxxxxxxxxxx>
CC: Patrick McHardy <kaber@xxxxxxxxx>
CC: Andrey Vagin <avagin@xxxxxxxxxx>
CC: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Cyrill Gorcunov <gorcunov@xxxxxxxxxx>

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
--- linux-ml.git.orig/include/net/raw.h
+++ linux-ml.git/include/net/raw.h
@@ -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
--- linux-ml.git.orig/include/net/rawv6.h
+++ linux-ml.git/include/net/rawv6.h
@@ -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
--- linux-ml.git.orig/include/uapi/linux/inet_diag.h
+++ linux-ml.git/include/uapi/linux/inet_diag.h
@@ -38,7 +38,10 @@ struct inet_diag_req_v2 {
__u8 sdiag_family;
__u8 sdiag_protocol;
__u8 idiag_ext;
- __u8 pad;
+ 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?