netdev - Re: [PATCH 1/2] genl: Fix genl dumpit() locking.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 23 Aug 2013 09:20:32 +0200
From:	Johannes Berg <johannes@...solutions.net>
To:	Pravin B Shelar <pshelar@...ira.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Jesse Gross <jesse@...ira.com>
Subject: Re: [PATCH 1/2] genl: Fix genl dumpit() locking.


> @@ -572,15 +594,29 @@ static int genl_family_rcv_msg(struct
> genl_family *family,
>                 return -EPERM;
>  
>         if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
> -               struct netlink_dump_control c = {
> -                       .dump = ops->dumpit,
> -                       .done = ops->done,
> -               };
> +               struct netlink_dump_control c;
> +               int rc;
>  
>                 if (ops->dumpit == NULL)
>                         return -EOPNOTSUPP;
>  
> -               return netlink_dump_start(net->genl_sock, skb, nlh,
> &c);
> +               memset(&c, 0, sizeof(c));
> +               if (!family->parallel_ops) {
> +                       genl_unlock();
> +                       c.data = ops;
> +                       c.dump = genl_lock_dumpit;
> +                       if (ops->done)
> +                               c.done = genl_lock_done;
> +               } else {
> +                       c.dump = ops->dumpit;
> +                       c.done = ops->done;
> +               }
> +
> +               rc = netlink_dump_start(net->genl_sock, skb, nlh, &c);
> +               if (!family->parallel_ops)
> +                       genl_lock();
> +               return rc;
> +

I think this piece would be easier to read if you call
netlink_dump_start() separately in the two branches. If you also move
the "c" variable into the branches then you can keep initializing it
with an explicit initializer which would also be more readable IMHO.

Either way, this seems fine. I still think that not assigning cb_mutex
wasn't the smartest idea, but I'll reply over in the other thread.

johannes


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html