From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] dma-buf: mmap support Date: Tue, 24 Apr 2012 19:02:44 +0200 Message-ID: <20120424170244.GD2017@phenom.ffwll.local> References: <1335258532-20739-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: InKi Dae Cc: Daniel Vetter , linaro-mm-sig@lists.linaro.org, LKML , DRI Development , Rob Clark , Rebecca Schultz Zavin , linux-media@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On Wed, Apr 25, 2012 at 01:37:51AM +0900, InKi Dae wrote: > Hi, >=20 > > > > +static int dma_buf_mmap_internal(struct file *file, struct vm_area= _struct *vma) > > +{ > > + =A0 =A0 =A0 struct dma_buf *dmabuf; > > + > > + =A0 =A0 =A0 if (!is_dma_buf_file(file)) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > > + > > + =A0 =A0 =A0 dmabuf =3D file->private_data; > > + > > + =A0 =A0 =A0 /* check for overflowing the buffer's size */ > > + =A0 =A0 =A0 if (vma->vm_pgoff + ((vma->vm_end - vma->vm_start) >>= PAGE_SHIFT) > > > + =A0 =A0 =A0 =A0 =A0 dmabuf->size >> PAGE_SHIFT) >=20 > is this condition right? your intention is for checking buffer's size > is valid or not. by the way why is vma->vm_pgoff added to vm region > size? This check here is to ensure that userspace cannot mmap beyong the end = of the dma_buf object. vm_pgoff is the offset userspace passed in at mmap time and hence needs to be added. Note that vm_end and vm_start are in bytes, wheres vm_pgoff is in pages. > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > > + > > + =A0 =A0 =A0 return dmabuf->ops->mmap(dmabuf, vma); > > +} > > + > > =A0static const struct file_operations dma_buf_fops =3D { > > =A0 =A0 =A0 =A0.release =A0 =A0 =A0 =A0=3D dma_buf_release, > > + =A0 =A0 =A0 .mmap =A0 =A0 =A0 =A0 =A0 =3D dma_buf_mmap_internal, > > =A0}; > > > > =A0/* > > @@ -82,7 +100,8 @@ struct dma_buf *dma_buf_export(void *priv, const= struct dma_buf_ops *ops, > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| !ops->unmap_d= ma_buf > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| !ops->release > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0|| !ops->kmap_at= omic > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || !ops->kmap)) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || !ops->kmap > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || !ops->mmap)) { > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return ERR_PTR(-EINVAL); > > =A0 =A0 =A0 =A0} > > > > @@ -406,3 +425,46 @@ void dma_buf_kunmap(struct dma_buf *dmabuf, un= signed long page_num, > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dmabuf->ops->kunmap(dmabuf, page_num= , vaddr); > > =A0} > > =A0EXPORT_SYMBOL_GPL(dma_buf_kunmap); > > + > > + > > +/** > > + * dma_buf_mmap - Setup up a userspace mmap with the given vma > > + * @dma_buf: =A0 [in] =A0 =A0buffer that should back the vma > > + * @vma: =A0 =A0 =A0 [in] =A0 =A0vma for the mmap > > + * @pgoff: =A0 =A0 [in] =A0 =A0offset in pages where this mmap sho= uld start within the > > + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dma-buf buffer. > > + * > > + * This function adjusts the passed in vma so that it points at th= e file of the > > + * dma_buf operation. It alsog adjusts the starting pgoff and does= bounds > > + * checking on the size of the vma. Then it calls the exporters mm= ap function to > > + * set up the mapping. > > + * > > + * Can return negative error values, returns 0 on success. > > + */ > > +int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vm= a, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long pgoff) > > +{ > > + =A0 =A0 =A0 if (WARN_ON(!dmabuf || !vma)) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > > + > > + =A0 =A0 =A0 /* check for offset overflow */ > > + =A0 =A0 =A0 if (pgoff + ((vma->vm_end - vma->vm_start) >> PAGE_SH= IFT) < pgoff) >=20 > ditto. isn't it checked whether page offset to be mmaped is placed > within vm region or not with the condition, if ((vma->vm_end - > vma->vm_start) >> PAGE_SHIFT) < pgoff)? Nope, this check only checks for overflow. The pgoff is the offset with= in the dma_buf object. E.g. a drm driver splits up it mmap space into piec= es, which map to individual buffers. If userspace just mmaps parts of such = a buffer, the importer can pass the offset in pgoff. But I expect this to= be 0 for almost all cases. Note that we don't need this overflow check in the internal mmap functi= on because do_mmap will do it for us. But here the importer potentially se= ts a completely different pgoff, so we need to do it. dma_buf documentatio= n also mentions this (and that importers do not have to do these checks). Yours, Daniel >=20 > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EOVERFLOW; > > + > > + =A0 =A0 =A0 /* check for overflowing the buffer's size */ > > + =A0 =A0 =A0 if (pgoff + ((vma->vm_end - vma->vm_start) >> PAGE_SH= IFT) > > > + =A0 =A0 =A0 =A0 =A0 dmabuf->size >> PAGE_SHIFT) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EINVAL; > > + > > + =A0 =A0 =A0 /* readjust the vma */ > > + =A0 =A0 =A0 if (vma->vm_file) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fput(vma->vm_file); > > + > > + =A0 =A0 =A0 vma->vm_file =3D dmabuf->file; > > + =A0 =A0 =A0 get_file(vma->vm_file); > > + > > + =A0 =A0 =A0 vma->vm_pgoff =3D pgoff; > > + > > + =A0 =A0 =A0 return dmabuf->ops->mmap(dmabuf, vma); > > +} > > +EXPORT_SYMBOL_GPL(dma_buf_mmap); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > > index 3efbfc2..1f78d15 100644 > > --- a/include/linux/dma-buf.h > > +++ b/include/linux/dma-buf.h > > @@ -61,6 +61,10 @@ struct dma_buf_attachment; > > =A0* =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0This Callback must not sleep. > > =A0* @kmap: maps a page from the buffer into kernel address space. > > =A0* @kunmap: [optional] unmaps a page from the buffer. > > + * @mmap: used to expose the backing storage to userspace. Note th= at the > > + * =A0 =A0 =A0 mapping needs to be coherent - if the exporter does= n't directly > > + * =A0 =A0 =A0 support this, it needs to fake coherency by shootin= g down any ptes > > + * =A0 =A0 =A0 when transitioning away from the cpu domain. > > =A0*/ > > =A0struct dma_buf_ops { > > =A0 =A0 =A0 =A0int (*attach)(struct dma_buf *, struct device *, > > @@ -92,6 +96,8 @@ struct dma_buf_ops { > > =A0 =A0 =A0 =A0void (*kunmap_atomic)(struct dma_buf *, unsigned lon= g, void *); > > =A0 =A0 =A0 =A0void *(*kmap)(struct dma_buf *, unsigned long); > > =A0 =A0 =A0 =A0void (*kunmap)(struct dma_buf *, unsigned long, void= *); > > + > > + =A0 =A0 =A0 int (*mmap)(struct dma_buf *, struct vm_area_struct *= vma); > > =A0}; > > > > =A0/** > > @@ -167,6 +173,9 @@ void *dma_buf_kmap_atomic(struct dma_buf *, uns= igned long); > > =A0void dma_buf_kunmap_atomic(struct dma_buf *, unsigned long, void= *); > > =A0void *dma_buf_kmap(struct dma_buf *, unsigned long); > > =A0void dma_buf_kunmap(struct dma_buf *, unsigned long, void *); > > + > > +int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long); > > =A0#else > > > > =A0static inline struct dma_buf_attachment *dma_buf_attach(struct d= ma_buf *dmabuf, > > @@ -248,6 +257,13 @@ static inline void dma_buf_kunmap(struct dma_b= uf *dmabuf, > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= unsigned long pnum, void *vaddr) > > =A0{ > > =A0} > > + > > +static inline int dma_buf_mmap(struct dma_buf *dmabuf, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct= vm_area_struct *vma, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsign= ed long pgoff) > > +{ > > + =A0 =A0 =A0 return -ENODEV; > > +} > > =A0#endif /* CONFIG_DMA_SHARED_BUFFER */ > > > > =A0#endif /* __DMA_BUF_H__ */ > > -- > > 1.7.10 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel --=20 Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48