netdev - Re: [PATCH net-next v2] rocker: move netevent neigh update to processes context

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAE4R7bBfHeK8U_8-QWoVoaVAGT6ObemaKdGOon_nyO8gooyU4A@mail.gmail.com>
Date:	Thu, 4 Jun 2015 18:33:10 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	Simon Horman <simon.horman@...ronome.com>,
	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>,
	Jiří Pírko <jiri@...nulli.us>,
	Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Subject: Re: [PATCH net-next v2] rocker: move netevent neigh update to
 processes context

On Thu, Jun 4, 2015 at 3:54 PM, Simon Horman <horms@...ge.net.au> wrote:
> Hi Scott,
>
>> Simon, just focusing on this rocker->neigh_tbl_next_index++ issue at
>> the moment, I think the change below would make _rocker_neigh_add()
>> safe to call without needing to put neigh_update into process context.
>> It works because entry is stored between PREPARE and COMMIT, so once
>> entry->index is assigned in PREPARE (under neigh_tbl_lock), it'll be
>> re-used in COMMIT, even if the NONE thread also claims it's
>> entry->index (also under neigh_tbl_lock).
>>
>> I know there are other issues you and Toshiaki Makita have pointed
>> out, but let's see if we can peel the onion one layer at a time.
>>
>> --- a/drivers/net/ethernet/rocker/rocker.c
>> +++ b/drivers/net/ethernet/rocker/rocker.c
>> @@ -2904,10 +2904,10 @@ static void _rocker_neigh_add(struct rocker *rocker,
>>                               enum switchdev_trans trans,
>>                               struct rocker_neigh_tbl_entry *entry)
>>  {
>> -       entry->index = rocker->neigh_tbl_next_index;
>> +       if (trans != SWITCHDEV_TRANS_COMMIT)
>> +               entry->index = rocker->neigh_tbl_next_index++;
>>         if (trans == SWITCHDEV_TRANS_PREPARE)
>>                 return;
>> -       rocker->neigh_tbl_next_index++;
>>         entry->ref_count++;
>>         hash_add(rocker->neigh_tbl, &entry->entry,
>>                  be32_to_cpu(entry->ip_addr));
>
> I agree that should work.

I've been running it today with no problems.  So I'm preparing a new
patch that changes direction here, to address David's concern about
switching neigh event context.  Details:

1) Above little patch addresses the race issue with neigh_tbl_next_index

2) Keep neigh update event in event context and bring back a couple of
items I removed in the Spring Cleanup series:

   a) use ROCKER_OP_FLAG_NOWAIT to mark contexts that can't sleep
   b) if nowait, mem allocations use GFP_ATOMIC
   c) if nowait, cmd to device is issued, but we will not sleep to
wait for response from device; just hit-and-run

I was probably too aggressive with Spring Cleanup thinking we could
swing all device accesses to process context.

Thanks to all for the testing/review/feedback/patience.
--
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