linux-kernel - Re: [PATCH v5 2/2] media: rc: introduce Meson IR TX driver

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]

Date:   Fri, 16 Jul 2021 09:24:05 +0100
From:   Sean Young <sean@...s.org>
To:     Viktor Prutyanov <viktor.prutyanov@...stech.edu>
Cc:     mchehab@...nel.org, robh+dt@...nel.org, khilman@...libre.com,
        narmstrong@...libre.com, jbrunet@...libre.com,
        martin.blumenstingl@...glemail.com, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, rockosov@...il.com
Subject: Re: [PATCH v5 2/2] media: rc: introduce Meson IR TX driver

Hi Viktor,

On Fri, Jul 16, 2021 at 01:36:52AM +0300, Viktor Prutyanov wrote:
> Hi Sean,
> 
> On Thu, 15 Jul 2021 22:40:01 +0100
> Sean Young <sean@...s.org> wrote:
> 
> > On Thu, Jul 15, 2021 at 12:27:06AM +0300, Viktor Prutyanov wrote:
 > > +	meson_irtx_fill_buf(ir, tx_buf, buf, len);
> > > +	dev_dbg(ir->dev, "TX buffer filled, length = %u\n", len);
> > > +
> > > +	spin_lock_irqsave(&ir->lock, flags);
> > > +	meson_irtx_update_buf(ir, tx_buf, len, 0);
> > > +	reinit_completion(&ir->completion);
> > > +	meson_irtx_send_buffer(ir);
> > > +	spin_unlock_irqrestore(&ir->lock, flags);
> > > +
> > > +	ret = wait_for_completion_interruptible(&ir->completion);
> > > +	dev_dbg(ir->dev, "TX %s\n", ret ? "interrupted" :
> > > "completed");  
> > 
> > Here two things can happen. One is, the process received a signal
> > (e.g. ^C). The other is that the hardware didn't issue any interrupts
> > due some problem somewhere. In the latter case, we only escape this
> > wait_for_completion_interruptable() when the user gets fed up and
> > presses ^C or something like that.
> > 
> > > +
> > > +	spin_lock_irqsave(&ir->lock, flags);
> > > +	kfree(ir->buf);
> > > +	meson_irtx_update_buf(ir, NULL, 0, 0);
> > > +	spin_unlock_irqrestore(&ir->lock, flags);  
> > 
> > Now it is possible that the buffer gets cleared before that IR was
> > sent, if the signal was received early enough. This means not all the
> > Tx was completed.
> > 
> > > +
> > > +	return len;  
> > 
> > Yet, we always return success.
> > 
> > In case no interrupts were generated we should return an error in a
> > timely manner, so the wait_for_completion() needs the timeout. You
> > can use the fact that the IR is never longer IR_MAX_DURATION (half a
> > second currently). Not sure what the returned error should be, maybe
> > -ETIMEDOUT?
> 
> As for me, ETIMEDOUT is OK.
> > 
> > The problem with the interruptable wait is that a process can receive
> > a signal at any time, and now when this happens your IR gets
> > truncated. I don't think this is what you want.
> 
> Should I replace wait_for_completion_interruptible by
> wait_for_completion_timeout in order to wait in uninterruptible way?

Yes, the process can receive a signal if the terminal is resized
(SIGWINCH) or if the process is backgrounded and then foregrounded with
^Z and fg (SIGCONT). If this happens during tx then the tx might be
incomplete.

Thanks,

Sean