[PATCH v2 1/2] rtnl_link: decode ifinfomsg netlink attributes

[Dmitry V. Levin]

On Sun, Aug 20, 2017 at 08:15:47PM +0800, JingPiao Chen wrote:
> On Sun, Aug 20, 2017 at 12:47:56PM +0300, Dmitry V. Levin wrote:
> > On Sun, Aug 20, 2017 at 04:06:25PM +0800, JingPiao Chen wrote:
> > > On Sun, Aug 20, 2017 at 01:33:53AM +0300, Dmitry V. Levin wrote:
> > > > On Sat, Aug 19, 2017 at 09:50:10AM +0800, JingPiao Chen wrote:
> > > > > * configure.ac (AC_CHECK_HEADERS): Add linux/if_link.h.
> > > > > (AC_CHECK_TYPES): Check for struct rtnl_link_stats64 in linux/if_link.h.
> > > > > (AC_CHECK_MEMBERS): Check for rx_nohandler field
> > > > > in struct rtnl_link_stats and struct rtnl_link_stats64.
> > > > > * rtnl_link.c: Include <arpa/inet.h>, <linux/if_arp.h>,
> > > > > <linux/if_link.h> and <linux/netdevice.h>.
> > > > > (min_ifla_address_len, ifla_address_default_decoder,
> > > > > ifla_address_type_specific_decoder,
> > > > > decode_ifla_address, decode_rtnl_link_stats,
> > > > > decode_rtnl_link_ifmap, decode_rtnl_link_stats64,
> > > > > print_item_id, decode_ifla_phys_item_id): New functions.
> > > > > (decode_ifla_phys_item_id): New array.
> > > > 
> > > > How could decode_ifla_phys_item_id be both a function and an array?
> > > > 
> > > > > (decode_ifinfomsg): Use it.
> > > > > ---
> > > [...]
> > > > > +static bool
> > > > > +decode_rtnl_link_stats(struct tcb *const tcp,
> > > > > +		       const kernel_ulong_t addr,
> > > > > +		       const unsigned int len,
> > > > > +		       const void *const opaque_data)
> > > > > +{
> > > > > +	struct rtnl_link_stats st;
> > > > > +
> > > > > +	if (len < sizeof(st))
> > > > > +		return false;
> > > > 
> > > > The kernel may not transfer struct rtnl_link_stats.rx_nohandler despite
> > > > the latter being defined by linux/if_link.h, e.g. if the kernel uses
> > > > an older version of struct rtnl_link_stats.
> > > > The minimal size is therefore not sizeof(struct rtnl_link_stats)
> > > > but offsetofend(struct rtnl_link_stats, tx_compressed).
> > > > 
> > > > Likewise, with struct rtnl_link_stats64.rx_nohandler.
> > > 
> > > You means:
> > > 
> > > 	struct rtnl_link_stats st;
> > > 	const unsigned int sizeof_stat =
> > > 		offsetofend(struct rtnl_link_stats, tx_compressed);
> > > 
> > > 	if (len < sizeof_stat)
> > > 		return false;
> > > 	else if (!umoven_or_printaddr(tcp, addr, sizeof_stat, &st)) {
> > > 	...
> > > 
> > > I can not understand where I can know kernel not transfer
> > > struct rtnl_link_stats.rx_nohandler.
> > 
> > This can happen, for example, when kernel headers used to build strace
> > are newer when the kernel itself.
> 
> static bool
> decode_rtnl_link_stats(struct tcb *const tcp,
> 		       const kernel_ulong_t addr,
> 		       const unsigned int len,
> 		       const void *const opaque_data)
> {
> 	struct rtnl_link_stats st;
> 	const unsigned int sizeof_stat =
> 		offsetofend(struct rtnl_link_stats, tx_compressed);

I think sizeof_stat should better be called min_size.

> 	const unsigned int size = len < sizeof(st) ? sizeof_stat : sizeof(st);
> 
> 	if (len < sizeof(st) && len < sizeof_stat)

Just if (len < min_size)?

> 		return false;
> 	else if (!umoven_or_printaddr(tcp, addr, size, &st)) {
> 		...
> 
> #ifdef HAVE_STRUCT_RTNL_LINK_STATS_RX_NOHANDLER
> 		if (len > sizeof_stat)

Maybe if (len >= sizeof(st))?

> 			PRINT_FIELD_U(", ", st, rx_nohandler);
> #endif
> 		tprints("}");
> 	}
> 
> 	return true;
> }
> 
> This is ok?

Yes, more or less.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170820/379d6feb/attachment.bin>