netdev - Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling

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:   Tue, 3 Oct 2017 19:48:53 +0000
From:   "Kalderon, Michal" <Michal.Kalderon@...ium.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>
Subject: Re: [PATCH v2 net-next 06/12] qed: Add LL2 slowpath handling

From: Leon Romanovsky <leon@...nel.org>
Sent: Tuesday, October 3, 2017 4:26 PM

>On Tue, Oct 03, 2017 at 11:54:56AM +0300, Michal Kalderon wrote:
>> For iWARP unaligned MPA flow, a slowpath event of flushing an
>> MPA connection that entered an unaligned state is required.
>> The flush ramrod is received on the ll2 queue, and a pre-registered
>> callback function is called to handle the flush event.
>>
>> Signed-off-by: Michal Kalderon <Michal.Kalderon@...ium.com>
>> Signed-off-by: Ariel Elior <Ariel.Elior@...ium.com>
>> ---
>>  drivers/net/ethernet/qlogic/qed/qed_ll2.c | 40 +++++++++++++++++++++++++++++--
>>  include/linux/qed/qed_ll2_if.h            |  5 ++++
>>  2 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> index 8eb9645..047f556 100644
>> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
>> @@ -423,6 +423,41 @@ static void qed_ll2_rxq_parse_reg(struct qed_hwfn *p_hwfn,
>>  }
>>
>>  static int
>> +qed_ll2_handle_slowpath(struct qed_hwfn *p_hwfn,
>> +                     struct qed_ll2_info *p_ll2_conn,
>> +                     union core_rx_cqe_union *p_cqe,
>> +                     unsigned long *p_lock_flags)
>> +{
>> +     struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
>> +     struct core_rx_slow_path_cqe *sp_cqe;
>> +
>> +     sp_cqe = &p_cqe->rx_cqe_sp;
>> +     if (sp_cqe->ramrod_cmd_id != CORE_RAMROD_RX_QUEUE_FLUSH) {
>> +             DP_NOTICE(p_hwfn,
>> +                       "LL2 - unexpected Rx CQE slowpath ramrod_cmd_id:%d\n",
>> +                       sp_cqe->ramrod_cmd_id);
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (!p_ll2_conn->cbs.slowpath_cb) {
>> +             DP_NOTICE(p_hwfn,
>> +                       "LL2 - received RX_QUEUE_FLUSH but no callback was provided\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     spin_unlock_irqrestore(&p_rx->lock, *p_lock_flags);
>
>Interesting, you are unlock the lock which was taken in upper layer.
>It is not actual error, but chances to have such error are pretty high
>(for example, after refactoring).

Thanks. Ensuring that the lock will only be unlocked inside the calling function would make 
the calling function long and less readable.
The risk exists, but I think the fact that p_lock_flags is passed as parameter should 
give a strong indication in the future that lock should be handled delicately.