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] [day] [month] [year] [list]

Message-ID: <CALnjE+qnDQazprpm2Y8iiQ8USuY7J3LX1GooYp4xN2JZ5FW9+Q@mail.gmail.com>
Date:	Fri, 23 Aug 2013 10:05:10 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Jesse Gross <jesse@...ira.com>
Subject: Re: [PATCH 1/2] genl: Fix genl dumpit() locking.

On Fri, Aug 23, 2013 at 12:20 AM, Johannes Berg
<johannes@...solutions.net> wrote:
>
>> @@ -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.
>
ok, I will update patch.

> 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.
--
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