linux-kernel - Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c

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: <160071917655.4188128.4175000228517858211@swboyd.mtv.corp.google.com>
Date:   Mon, 21 Sep 2020 13:12:56 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     rojay@...eaurora.org
Cc:     wsa@...nel.org, dianders@...omium.org,
        saiprakash.ranjan@...eaurora.org, gregkh@...uxfoundation.org,
        mka@...omium.org, akashast@...eaurora.org,
        msavaliy@....qualcomm.com, skakit@...eaurora.org,
        vkaur@...eaurora.org, pyarlaga@...eaurora.org,
        rnayak@...eaurora.org, agross@...nel.org,
        bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        sumit.semwal@...aro.org, linux-media@...r.kernel.org
Subject: Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c

Quoting rojay@...eaurora.org (2020-09-21 04:21:04)
> Hi Stephen,
> 
> On 2020-09-18 01:53, Stephen Boyd wrote:
> > Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
> >> b/drivers/i2c/busses/i2c-qcom-geni.c
> >> index dead5db3315a..b0d8043c8cb2 100644
> >> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> >>  };
> >> 
> >>  struct geni_i2c_err_log {
> >> @@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct 
> >> geni_i2c_dev *gi2c)
> >> 
> >>         spin_lock_irqsave(&gi2c->lock, flags);
> >>         geni_i2c_err(gi2c, GENI_TIMEOUT);
> >> -       gi2c->cur = NULL;
> > 
> > This looks concerning. We're moving this out from under the spinlock.
> > The irq handler in this driver seems to hold the spinlock all the time
> > while processing and this function grabs it here to keep cur consistent
> > when aborting the transfer due to a timeout. Otherwise it looks like 
> > the
> > irqhandler can race with this and try to complete the transfer while
> > it's being torn down here.
> > 
> >>         geni_se_abort_m_cmd(&gi2c->se);
> >>         spin_unlock_irqrestore(&gi2c->lock, flags);
> >>         do {
> >> @@ -349,10 +352,62 @@ static void geni_i2c_tx_fsm_rst(struct 
> >> geni_i2c_dev *gi2c)
> >>                 dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
> >>  }
> >> 
> >> +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c)
> > 
> > So maybe pass cur to this function?
> > 
> 
> Sorry, i did not understand why to pass cur to this function?

I'm suggesting to copy the cur data out of the gi2c pointer and then
pass it to these functions so that it can't race with another transfer.
Something like an atomic exchange may work. I haven't thought about it
deeply, but we need to make sure the irq handler can't race with the
cleanup functions.