[PATCH] netlink: introduce a dummy netlink attributes parser
JingPiao Chen
chenjingpiao at gmail.com
Sat Jun 17 04:43:41 UTC 2017
On Sat, Jun 17, 2017 at 12:22:48AM +0300, Dmitry V. Levin wrote:
> On Fri, Jun 16, 2017 at 01:35:07PM +0800, JingPiao Chen wrote:
> > * defs.h (decode_nlattr): New prototype.
> > * linux/unix_diag.h (UNIX_DIAG_*): New enum.
> > * netlink.c (fetch_nlattr, print_nlattr,
> > decode_nlattr_with_data, decode_nlattr): New functions.
>
> Looks like there is no necessity to place these new nlattr related
> functions into netlink.c, what do you think about creating a new source
> file for this purpose, e.g. nlattr.c?
>
I like create new file nalttr.c.
> > * netlink_sock_diag.c: Include "xlat/unix_diag_attrs.h".
> > (decode_unix_diag_msg): Use decode_nlattr.
> > * xlat/unix_diag_attrs.in: New file.
> >
> > co-authored-by: Fabien Siron <fabien.siron at epita.fr>
> > ---
> > defs.h | 3 ++
> > linux/unix_diag.h | 11 +++++--
> > netlink.c | 76
+++++++++++++++++++++++++++++++++++++++++++++++++
> > netlink_sock_diag.c | 4 +++
> > xlat/unix_diag_attrs.in | 7 +++++
> > 5 files changed, 99 insertions(+), 2 deletions(-)
> > create mode 100644 xlat/unix_diag_attrs.in
> >
> > diff --git a/defs.h b/defs.h
> > index 487e51b..7e3912d 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -632,6 +632,9 @@ tprint_iov_upto(struct tcb *, kernel_ulong_t len,
kernel_ulong_t addr,
> >
> > extern void
> > decode_netlink(struct tcb *, int fd, kernel_ulong_t addr,
kernel_ulong_t len);
> > +extern void
> > +decode_nlattr(struct tcb *tcp, kernel_ulong_t addr, kernel_ulong_t len,
> > + int, const struct xlat *, const char *);
> >
> > extern void tprint_open_modes(unsigned int);
> > extern const char *sprint_open_modes(unsigned int);
> > diff --git a/linux/unix_diag.h b/linux/unix_diag.h
> > index a6b62ba..3c9d99f 100644
> > --- a/linux/unix_diag.h
> > +++ b/linux/unix_diag.h
> > @@ -27,7 +27,14 @@ struct unix_diag_msg {
> > uint32_t udiag_cookie[2];
> > };
> >
> > -#define UNIX_DIAG_NAME 0
> > -#define UNIX_DIAG_PEER 2
> > +enum {
> > + UNIX_DIAG_NAME,
> > + UNIX_DIAG_VFS,
> > + UNIX_DIAG_PEER,
> > + UNIX_DIAG_ICONS,
> > + UNIX_DIAG_RQLEN,
> > + UNIX_DIAG_MEMINFO,
> > + UNIX_DIAG_SHUTDOWN,
> > +};
> >
> > #endif /* !STRACE_LINUX_UNIX_DIAG_H */
> > diff --git a/netlink.c b/netlink.c
> > index 4bef949..52f3806 100644
> > --- a/netlink.c
> > +++ b/netlink.c
> > @@ -49,6 +49,82 @@
> > #undef NLMSG_HDRLEN
> > #define NLMSG_HDRLEN NLMSG_ALIGN(sizeof(struct nlmsghdr))
> >
> > +static bool
> > +fetch_nlattr(struct tcb *tcp, struct nlattr *nlattr,
>
> You can add some const qualifiers here, like in fetch_nlmsghdr that was
> used as a template for this new function.
>
> > + const kernel_ulong_t addr, const kernel_ulong_t len)
> > +{
> > + if (len < sizeof(struct nlattr)) {
> > + printstrn(tcp, addr, len);
> > + return false;
> > + }
> > +
> > + if (umove_or_printaddr(tcp, addr, nlattr))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void
> > +print_nlattr(const struct nlattr *const nla,
> > + const struct xlat *table, const char *dflt)
> > +{
> > + tprintf("{nla_len=%u, nla_type=", nla->nla_len);
> > + if (nla->nla_type & NLA_F_NESTED)
> > + tprints("NLA_F_NESTED|");
> > + if (nla->nla_type & NLA_F_NET_BYTEORDER)
> > + tprints("NLA_F_NET_BYTEORDER|");
> > + printxval(table, nla->nla_type & NLA_TYPE_MASK, dflt);
> > + tprints("}");
> > +}
> > +
> > +static void
> > +decode_nlattr_with_data(struct tcb *tcp, kernel_ulong_t addr,
> > + kernel_ulong_t len, const struct xlat *table,
> > + const char *dflt)
> > +{
> > + struct nlattr nlattr;
> > +
> > + while (fetch_nlattr(tcp, &nlattr, addr, len)) {
> > + unsigned long nlattr_len = NLA_ALIGN(nlattr.nla_len);
> > + kernel_ulong_t next_addr = 0;
> > + kernel_ulong_t next_len = 0;
>
> This new function is made after decode_netlink, which is fine,
> but it misses the abbrev(tcp) logic which is present in decode_netlink.
> Could you reintroduce it here, too, please?
>
> > +
> > + if (nlattr.nla_len >= sizeof(struct nlattr)) {
> > + next_len = (len >= nlattr_len) ? len - nlattr_len : 0;
> > +
> > + if (next_len && addr + nlattr_len > addr)
> > + next_addr = addr + nlattr_len;
> > + }
> > +
> > + tprints("{");
> > + print_nlattr(&nlattr, table, dflt);
> > + if (nlattr.nla_len > NLA_HDRLEN) {
> > + tprints(", ");
> > + printstrn(tcp, addr + NLA_HDRLEN,
> > + nlattr.nla_len - NLA_HDRLEN);
> > + }
> > + tprints("}");
> > +
> > + if (!next_addr)
> > + break;
> > +
> > + tprints(", ");
> > + addr = next_addr;
> > + len = next_len;
> > + }
> > +}
> > +
> > +void
> > +decode_nlattr(struct tcb *tcp, kernel_ulong_t addr, kernel_ulong_t len,
> > + int hdrlen, const struct xlat *table, const char *dflt)
> > +{
> > + if (len > NLMSG_ALIGN(hdrlen)) {
> > + tprints(", ");
> > + decode_nlattr_with_data(tcp, addr + NLMSG_ALIGN(hdrlen),
> > + len - NLMSG_ALIGN(hdrlen), table, dflt);
> > + }
>
> I'm not sure we need this proxy function, the caller is capable of
performing
> the check and invoking the decoder with the right address and length.
Yes, but delete this function will add a lot of code,
and the check is scattered.
> > +}
> > +
> > /*
> > * Fetch a struct nlmsghdr from the given address.
> > */
> > diff --git a/netlink_sock_diag.c b/netlink_sock_diag.c
> > index 8dbfd07..6d79ba4 100644
> > --- a/netlink_sock_diag.c
> > +++ b/netlink_sock_diag.c
> > @@ -55,6 +55,7 @@
> > # include "xlat/smc_states.h"
> > #endif
> >
> > +#include "xlat/unix_diag_attrs.h"
> > #include "xlat/unix_diag_show.h"
> >
> > static void
> > @@ -109,32 +110,35 @@ static void
> > decode_unix_diag_msg(struct tcb *const tcp,
> > const struct nlmsghdr *const nlmsghdr,
> > const uint8_t family,
> > const kernel_ulong_t addr,
> > const kernel_ulong_t len)
> > {
> > struct unix_diag_msg msg = { .udiag_family = family };
> > const size_t offset = sizeof(msg.udiag_family);
> >
> > tprints("{udiag_family=");
> > printxval(addrfams, msg.udiag_family, "AF_???");
> >
> > tprints(", ");
> > if (len >= sizeof(msg)) {
> > if (!umoven_or_printaddr(tcp, addr + offset,
> > sizeof(msg) - offset,
> > (void *) &msg + offset)) {
> > tprints("udiag_type=");
> > printxval(socktypes, msg.udiag_type, "SOCK_???");
> > tprintf(", udiag_state=");
> > printxval(tcp_states, msg.udiag_state, "TCP_???");
> > tprintf(", udiag_ino=%" PRIu32
> > ", udiag_cookie=[%" PRIu32 ", %" PRIu32 "]",
> > msg.udiag_ino,
> > msg.udiag_cookie[0], msg.udiag_cookie[1]);
>
> The invocation of nlattr decoder should be placed here:
>
> offset = NLMSG_ALIGN(sizeof(msg));
> if (len > offset) {
> tprints(", ");
> decode_nlattr(tcp, addr + offset, len - offset,
> unix_diag_attrs, "UNIX_DIAG_???");
> }
> > }
> > } else
> > tprints("...");
> > tprints("}");
> > +
> > + decode_nlattr(tcp, addr, len, sizeof(msg), unix_diag_attrs,
> > + "UNIX_DIAG_???");
> > }
> >
> > static void
> > diff --git a/xlat/unix_diag_attrs.in b/xlat/unix_diag_attrs.in
> > new file mode 100644
> > index 0000000..0fb2f23
> > --- /dev/null
> > +++ b/xlat/unix_diag_attrs.in
> > @@ -0,0 +1,7 @@
> > +UNIX_DIAG_NAME 0
> > +UNIX_DIAG_VFS 1
> > +UNIX_DIAG_PEER 2
> > +UNIX_DIAG_ICONS 3
> > +UNIX_DIAG_RQLEN 4
> > +UNIX_DIAG_MEMINFO 5
> > +UNIX_DIAG_SHUTDOWN 6
>
> As these constants are guaranteed to be defined by local linux/unix_diag.h
> file, there is no need to provide fallback definitions, just the opposite,
> these xlat entries should be unconditional:
>
> --- /dev/null
> +++ b/xlat/unix_diag_attrs.in
> @@ -0,0 +1,8 @@
> +#unconditional
> +UNIX_DIAG_NAME
> +UNIX_DIAG_VFS
> +UNIX_DIAG_PEER
> +UNIX_DIAG_ICONS
> +UNIX_DIAG_RQLEN
> +UNIX_DIAG_MEMINFO
> +UNIX_DIAG_SHUTDOWN
--
JingPiao Chen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170617/7974c1c7/attachment.html>
More information about the Strace-devel
mailing list