linux-kernel - Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine

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]

Message-ID: <20150811100617.GD7576@n2100.arm.linux.org.uk>
Date:	Tue, 11 Aug 2015 11:06:17 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Vinod Koul <vinod.koul@...el.com>
Cc:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	peter@...leysoftware.com, Dan Williams <dan.j.williams@...el.com>,
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
	nsekhar@...com, linux-omap@...r.kernel.org,
	linux-serial@...r.kernel.org, john.ogness@...utronix.de,
	Peter Ujfalusi <peter.ujfalusi@...com>
Subject: Re: [PATCH 2/3] dma: add __must_check annotation for
 dmaengine_pause()

On Tue, Aug 11, 2015 at 03:28:52PM +0530, Vinod Koul wrote:
> On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
> > In 8250-omap I learned it the hard way that ignoring the return code
> > of dmaengine_pause() might be bad because the underlying DMA driver
> > might not support the function at all and so not doing what one is
> > expecting.
> > This patch adds the __must_check annotation as suggested by Russell King.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > ---
> >  include/linux/dmaengine.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 8ad9a4e839f6..4eac4716bded 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
> >  	return -ENOSYS;
> >  }
> >  
> > -static inline int dmaengine_pause(struct dma_chan *chan)
> > +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> >  {
> >  	if (chan->device->device_pause)
> >  		return chan->device->device_pause(chan);
> 
> Give that there are bunch of users of this call which may or maynot be using
> this, I think putting this check is too stringent

I think what people need to learn is that an API in the kernel which
returns an int _can_ fail - it returns an int so it _can_ return an
error code.  If it _can_ return an error code, there _will_ be
implementations which _do_.

If you don't check the return code, either your code doesn't care whether
the function was successful or not, or you're playing with fire.  This is
a prime example of playing with fire.

Let's leave the crappy userspace laziness with regard to error checking
to userspace, and keep it out of the kernel.

Yes, the DMA engine capabilities may not be sufficient to describe every
detail of DMA engines, but that's absolutely no reason to skimp on error
checking.  Had there been some kind of error checking at the site, this
problem would have been spotted before the 8250-omap driver was merged.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/