[Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[David Butterfield]

On 6/3/19 11:43 PM, Lars Ellenberg wrote:
> Think again: how is family->ops inexed?

If you mean the genl_family, its ops are kept on a list, which is searched using genl_get_cmd().
Constructed as a list, it doesn't even (necessarily) have an an underlying array one might be tempted to index.

> How is drbd_genl_ops indexed?

It is an array, but it isn't indexed by command number, except in this one place in drbd_adm_prepare().

The array is generated by the GENL_op() macros in drbd-headers/linux/drbd_genl.h, with commands in no obvious order.
The first one is command number 5:
	GENL_op(DRBD_ADM_NEW_MINOR, 5, GENL_doit(drbd_adm_new_minor),

There are 36 GENL_op entries in drbd_genl.h, resulting in 36 entries in the drbd_genl_ops[] array.
But the highest command number is 47, so indexing by command number is liable to exceed the bounds of the array.

For example, here's another GENL_op() entry from drbd_genl.h, which appears at drbd_genl_ops[5]:
	GENL_op(DRBD_ADM_NEW_PEER, 44, GENL_doit(drbd_adm_new_peer),

This command attempts to access drbd_genl_ops[44], leading to a runtime error at drbd_nl.c:222:
	drbdsetup new-peer r0 1 --_name=vagrant

drbd_nl.c:222:20: runtime error: index 44 out of bounds for type 'genl_ops [36]'
drbd_nl.c:222:25: runtime error: load of address 0x0000010430e4 with insufficient space for an object of type 'unsigned int'
0x0000010430e4: note: pointer points here
  70 39 c2 00 00 00 00 00  6d 00 00 00 05 00 00 00  5c 2f 04 01 00 00 00 00  03 03 00 00 00 00 00 00
              ^ 

 222         if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
 223             drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
 224                 return -EPERM;

Why does one need to be iterated over and compare a member with the search key and the other does not?

I do not understand that to be the case.  It looks like the definition of GENL_op() for ZZZ_genl_ops[]
in genl_magic_func.h does not record the op number into the generated entries.  I guess that will need
to be added for comparison with the search key.

Regards,
Dave Butterfield

> David Butterfield <dab21774 at gmail.com <mailto:dab21774 at gmail.com>> schrieb am Di., 4. Juni 2019, 01:58:
> 
>     (I tried "drbd-dev at lists.linbit.com <mailto:drbd-dev at lists.linbit.com>" first, but message never made it into the archive.)
> 
> 
>     -------- Forwarded Message --------
>     To: drbd-dev at lists.linbit.com <mailto:drbd-dev at lists.linbit.com>
>     From: David Butterfield <dab21774 at gmail.com <mailto:dab21774 at gmail.com>>
>     Subject: drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number
>     Date: Fri, 31 May 2019 13:01:24 -0600
>     User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1
>     MIME-Version: 1.0
>     Content-Type: text/plain; charset=utf-8
> 
>     (Is this the right place to send comments on the source code such as this one?)
> 
>     In drbd_nl.c:
> 
>       static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
>             struct sk_buff *skb, struct genl_info *info, unsigned flags)
>       {
>             struct drbd_genlmsghdr *d_in = info->userhdr;
>             const u8 cmd = info->genlhdr->cmd;
>             int err;
> 
>             memset(adm_ctx, 0, sizeof(*adm_ctx));
> 
>     +       //XXX I do not think you can find the ops for a command number by indexing this array.
>     +       //XXX The array is unordered and packed.  I think it must search like genl_get_cmd().
>             /*
>              * genl_rcv_msg() only checks if commands with the GENL_ADMIN_PERM flag
>              * set have CAP_NET_ADMIN; we also require CAP_SYS_ADMIN for
>              * administrative commands.
>              */
>             if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
>                 drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
>                     return -EPERM;
> 
>             adm_ctx->reply_skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>             if (!adm_ctx->reply_skb) {
>                     err = -ENOMEM;
>                     goto fail;
>             }
>