Re: [PATCH] wext: optimise, comment and fix event sending

["Luis R. Rodriguez" <mcgrof@gmail.com>]

On Fri, Jun 19, 2009 at 7:28 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> The current function for sending events first allocates the
> event stream buffer, and then an skb to copy the event stream
> into. This can be done in one go. Also, the current function
> leaks kernel data to userspace in a 4 uninitialised bytes,
> initialise those explicitly. Finally also add a few useful
> comments, as opposed to the current comments.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  net/wireless/wext.c |  114 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 57 insertions(+), 57 deletions(-)
>
> --- wireless-testing.orig/net/wireless/wext.c   2009-06-19 08:01:29.000000000 +0200
> +++ wireless-testing/net/wireless/wext.c        2009-06-19 16:27:57.000000000 +0200
> @@ -1300,22 +1300,15 @@ static void wireless_nlevent_process(str
>
>  static DECLARE_WORK(wireless_nlevent_work, wireless_nlevent_process);
>
> -/* ---------------------------------------------------------------- */
> -/*
> - * Fill a rtnetlink message with our event data.
> - * Note that we propage only the specified event and don't dump the
> - * current wireless config. Dumping the wireless config is far too
> - * expensive (for each parameter, the driver need to query the hardware).
> - */
> -static int rtnetlink_fill_iwinfo(struct sk_buff *skb, struct net_device *dev,
> -                                int type, char *event, int event_len)
> +static struct nlmsghdr *rtnetlink_ifinfo_prep(struct net_device *dev,
> +                                             struct sk_buff *skb)
>  {
>        struct ifinfomsg *r;
>        struct nlmsghdr  *nlh;
>
> -       nlh = nlmsg_put(skb, 0, 0, type, sizeof(*r), 0);
> -       if (nlh == NULL)
> -               return -EMSGSIZE;
> +       nlh = nlmsg_put(skb, 0, 0, RTM_NEWLINK, sizeof(*r), 0);
> +       if (!nlh)
> +               return NULL;
>
>        r = nlmsg_data(nlh);
>        r->ifi_family = AF_UNSPEC;
> @@ -1326,45 +1319,14 @@ static int rtnetlink_fill_iwinfo(struct
>        r->ifi_change = 0;      /* Wireless changes don't affect those flags */
>
>        NLA_PUT_STRING(skb, IFLA_IFNAME, dev->name);
> -       /* Add the wireless events in the netlink packet */
> -       NLA_PUT(skb, IFLA_WIRELESS, event_len, event);
> -
> -       return nlmsg_end(skb, nlh);
>
> -nla_put_failure:
> +       return nlh;
> + nla_put_failure:
>        nlmsg_cancel(skb, nlh);
> -       return -EMSGSIZE;
> +       return NULL;
>  }
>
> -/* ---------------------------------------------------------------- */
> -/*
> - * Create and broadcast and send it on the standard rtnetlink socket
> - * This is a pure clone rtmsg_ifinfo() in net/core/rtnetlink.c
> - * Andrzej Krzysztofowicz mandated that I used a IFLA_XXX field
> - * within a RTM_NEWLINK event.
> - */
> -static void rtmsg_iwinfo(struct net_device *dev, char *event, int event_len)
> -{
> -       struct sk_buff *skb;
> -       int err;
> -
> -       skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> -       if (!skb)
> -               return;
> -
> -       err = rtnetlink_fill_iwinfo(skb, dev, RTM_NEWLINK, event, event_len);
> -       if (err < 0) {
> -               WARN_ON(err == -EMSGSIZE);
> -               kfree_skb(skb);
> -               return;
> -       }
> -
> -       NETLINK_CB(skb).dst_group = RTNLGRP_LINK;
> -       skb_queue_tail(&dev_net(dev)->wext_nlevents, skb);
> -       schedule_work(&wireless_nlevent_work);
> -}
>
> -/* ---------------------------------------------------------------- */
>  /*
>  * Main event dispatcher. Called from other parts and drivers.
>  * Send the event on the appropriate channels.
> @@ -1383,6 +1345,9 @@ void wireless_send_event(struct net_devi
>        int wrqu_off = 0;                       /* Offset in wrqu */
>        /* Don't "optimise" the following variable, it will crash */
>        unsigned        cmd_index;              /* *MUST* be unsigned */
> +       struct sk_buff *skb;
> +       struct nlmsghdr *nlh;
> +       struct nlattr *nla;
>
>        /* Get the description of the Event */
>        if (cmd <= SIOCIWLAST) {
> @@ -1430,25 +1395,60 @@ void wireless_send_event(struct net_devi
>        hdr_len = event_type_size[descr->header_type];
>        event_len = hdr_len + extra_len;
>
> -       /* Create temporary buffer to hold the event */
> -       event = kmalloc(event_len, GFP_ATOMIC);

Hm, for stable purposes can this be fixed with a kzalloc here? If so
can this patch be split it up in 2 for that purpose?

  Luis