Hi,
This series fixes up the cases where callers of ksize() use it to
opportunistically grow their buffer sizes, which can run afoul of the
__alloc_size hinting that CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE
use to perform dynamic buffer bounds checking. Quoting the first patch:
In the effort to help the compiler reason about buffer sizes, the
__alloc_size attribute was added to allocators. This improves the scope
of the compiler's ability to apply CONFIG_UBSAN_BOUNDS and (in the near
future) CONFIG_FORTIFY_SOURCE. For most allocations, this works well,
as the vast majority of callers are not expecting to use more memory
than what they asked for.
There is, however, one common exception to this: anticipatory resizing
of kmalloc allocations. These cases all use ksize() to determine the
actual bucket size of a given allocation (e.g. 128 when 126 was asked
for). This comes in two styles in the kernel:
1) An allocation has been determined to be too small, and needs to be
resized. Instead of the caller choosing its own next best size, it
wants to minimize the number of calls to krealloc(), so it just uses
ksize() plus some additional bytes, forcing the realloc into the next
bucket size, from which it can learn how large it is now. For example:
data = krealloc(data, ksize(data) + 1, gfp);
data_len = ksize(data);
2) The minimum size of an allocation is calculated, but since it may
grow in the future, just use all the space available in the chosen
bucket immediately, to avoid needing to reallocate later. A good
example of this is skbuff's allocators:
data = kmalloc_reserve(size, gfp_mask, node, &pfmemalloc);
...
/* kmalloc(size) might give us more room than requested.
* Put skb_shared_info exactly at the end of allocated zone,
* to allow max possible filling before reallocation.
*/
osize = ksize(data);
size = SKB_WITH_OVERHEAD(osize);
In both cases, the "how large is the allocation?" question is answered
_after_ the allocation, where the compiler hinting is not in an easy place
to make the association any more. This mismatch between the compiler's
view of the buffer length and the code's intention about how much it is
going to actually use has already caused problems[1]. It is possible to
fix this by reordering the use of the "actual size" information.
We can serve the needs of users of ksize() and still have accurate buffer
length hinting for the compiler by doing the bucket size calculation
_before_ the allocation. Code can instead ask "how large an allocation
would I get for a given size?".
Introduce kmalloc_size_roundup(), to serve this function so we can start
replacing the "anticipatory resizing" uses of ksize().
[1] https://github.com/ClangBuiltLinux/linux/issues/1599https://github.com/KSPP/linux/issues/183
-------
And after adding kmalloc_size_roundup(), put it to use with the various
ksize() callers, restore the previously removed __alloc_size hint,
and fix the use of __malloc annotations.
I tried to trim the CC list on this series since it got rather long. I
kept all the suggested mailing lists, though. :)
Thanks!
-Kees
Kees Cook (12):
slab: Introduce kmalloc_size_roundup()
skbuff: Proactively round up to kmalloc bucket size
net: ipa: Proactively round up to kmalloc bucket size
btrfs: send: Proactively round up to kmalloc bucket size
dma-buf: Proactively round up to kmalloc bucket size
coredump: Proactively round up to kmalloc bucket size
igb: Proactively round up to kmalloc bucket size
openvswitch: Proactively round up to kmalloc bucket size
x86/microcode/AMD: Track patch allocation size explicitly
iwlwifi: Track scan_cmd allocation size explicitly
slab: Remove __malloc attribute from realloc functions
slab: Restore __alloc_size attribute to __kmalloc_track_caller
arch/x86/include/asm/microcode.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 3 +-
drivers/dma-buf/dma-resv.c | 9 +++-
drivers/net/ethernet/intel/igb/igb_main.c | 1 +
drivers/net/ipa/gsi_trans.c | 7 ++-
drivers/net/wireless/intel/iwlwifi/dvm/dev.h | 1 +
drivers/net/wireless/intel/iwlwifi/dvm/scan.c | 10 +++-
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 3 +-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 3 +-
drivers/net/wireless/intel/iwlwifi/mvm/scan.c | 6 +--
fs/btrfs/send.c | 11 +++--
fs/coredump.c | 7 ++-
include/linux/compiler_types.h | 13 ++----
include/linux/slab.h | 46 ++++++++++++++++---
mm/slab_common.c | 17 +++++++
net/core/skbuff.c | 34 +++++++-------
net/openvswitch/flow_netlink.c | 4 +-
17 files changed, 125 insertions(+), 51 deletions(-)
--
2.34.1
On 9/20/22 7:05 PM, Sean Anderson wrote:
>
>
> On 9/20/22 6:49 PM, Leo Li wrote:
>>
>>
>>> -----Original Message-----
>>> From: Sean Anderson <sean.anderson(a)seco.com>
>>> Sent: Tuesday, September 20, 2022 11:21 AM
>>> To: Robin Murphy <robin.murphy(a)arm.com>; Oleksij Rempel
>>> <linux(a)rempel-privat.de>; Pengutronix Kernel Team
>>> <kernel(a)pengutronix.de>; linux-i2c(a)vger.kernel.org; linux-arm-kernel
>>> <linux-arm-kernel(a)lists.infradead.org>; Vinod Koul <vkoul(a)kernel.org>;
>>> dmaengine(a)vger.kernel.org; Leo Li <leoyang.li(a)nxp.com>; Laurentiu Tudor
>>> <laurentiu.tudor(a)nxp.com>
>>> Cc: Linux Kernel Mailing List <linux-kernel(a)vger.kernel.org>; dri-
>>> devel(a)lists.freedesktop.org; Christian König <christian.koenig(a)amd.com>;
>>> linaro-mm-sig(a)lists.linaro.org; Shawn Guo <shawnguo(a)kernel.org>; Sumit
>>> Semwal <sumit.semwal(a)linaro.org>; Joy Zou <joy.zou(a)nxp.com>; linux-
>>> media(a)vger.kernel.org
>>> Subject: Re: [BUG] ls1046a: eDMA does not transfer data from I2C
>>>
>>>
>>>
>>> On 9/20/22 11:44 AM, Sean Anderson wrote:
>>> >
>>> >
>>> > On 9/20/22 11:24 AM, Sean Anderson wrote:
>>> >>
>>> >>
>>> >> On 9/20/22 6:07 AM, Robin Murphy wrote:
>>> >>> On 2022-09-19 23:24, Sean Anderson wrote:
>>> >>>> Hi all,
>>> >>>>
>>> >>>> I discovered a bug in either imx_i2c or fsl-edma on the LS1046A
>>> >>>> where no data is read in i2c_imx_dma_read except for the last two
>>> >>>> bytes (which are not read using DMA). This is perhaps best
>>> >>>> illustrated with the following example:
>>> >>>>
>>> >>>> # hexdump -C /sys/bus/nvmem/devices/0-00540/nvmem
>>> >>>> [ 308.914884] i2c i2c-0: ffff000809380000 0x0000000889380000
>>> 0x00000000f5401000 ffff000075401000
>>> >>>> [ 308.923529] src= 2180004 dst=f5401000 attr= 0 soff= 0 nbytes=1
>>> slast= 0
>>> >>>> [ 308.923529] citer= 7e biter= 7e doff= 1 dlast_sga= 0
>>> >>>> [ 308.923529] major_int=1 disable_req=1 enable_sg=0 [ 308.942113]
>>> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
>>> >>>> 00000000d9dd26c5[4]: submitted [ 308.974049] fsl-edma
>>> >>>> 2c00000.edma: txd 00000000d9dd26c5[4]: marked complete [
>>> >>>> 308.981339] i2c i2c-0: ffff000809380000 = [2e 2e 2f 2e 2e 2f 2e 2e
>>> >>>> 2f 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30
>>> 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34
>>> 30 00 00] [ 309.002226] i2c i2c-0: ffff000075401000 = [2e 2e 2f 2e 2e 2f 2e 2e 2f
>>> 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30 30 30 30
>>> 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00]
>>> [ 309.024649] i2c i2c-0: ffff000809380080 0x0000000889380080
>>> 0x00000000f5401800 ffff000075401800
>>> >>>> [ 309.033270] src= 2180004 dst=f5401800 attr= 0 soff= 0 nbytes=1
>>> slast= 0
>>> >>>> [ 309.033270] citer= 7e biter= 7e doff= 1 dlast_sga= 0
>>> >>>> [ 309.033270] major_int=1 disable_req=1 enable_sg=0 [ 309.051633]
>>> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
>>> >>>> 00000000d9dd26c5[5]: submitted [ 309.083526] fsl-edma
>>> >>>> 2c00000.edma: txd 00000000d9dd26c5[5]: marked complete [
>>> >>>> 309.090807] i2c i2c-0: ffff000809380080 = [00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00] [ 309.111694] i2c i2c-0:
>>> >>>> ffff000075401800 = [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> 00 00 00 00]
>>> >>>> 00000000 2e 2e 2f 2e 2e 2f 2e 2e 2f 64 65 76 69 63 65 73
>>> >>>> |../../../devices|
>>> >>>> 00000010 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31
>>> >>>> |/platform/soc/21|
>>> >>>> 00000020 38 30 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f
>>> >>>> |80000.i2c/i2c-0/|
>>> >>>> 00000030 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00
>>> >>>> |0-0054/0-00540..|
>>> >>>> 00000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> |................|
>>> >>>> *
>>> >>>> 00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff ff
>>> >>>> |................|
>>> >>>> 00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> >>>> |................|
>>> >>>> *
>>> >>>> 000000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff 5b
>>> >>>> |...............[|
>>> >>>> 00000100
>>> >>>>
>>> >>>> (patch with my debug prints appended below)
>>> >>>>
>>> >>>> Despite the DMA completing successfully, no data was copied into
>>> >>>> the buffer, leaving the original (now junk) contents. I probed the
>>> >>>> I2C bus with an oscilloscope, and I verified that the transfer did indeed
>>> occur.
>>> >>>> The timing between submission and completion seems reasonable for
>>> >>>> the bus speed (50 kHz for whatever reason).
>>> >>>>
>>> >>>> I had a look over the I2C driver, and nothing looked obviously
>>> >>>> incorrect. If anyone has ideas on what to try, I'm more than willing.
>>> >>>
>>> >>> Is the DMA controller cache-coherent? I see the mainline LS1046A DT
>>> doesn't have a "dma-coherent" property for it, but the behaviour is entirely
>>> consistent with that being wrong - dma_map_single() cleans the cache,
>>> coherent DMA write hits the still-present cache lines, dma_unmap_single()
>>> invalidates the cache, and boom, the data is gone and you read back the
>>> previous content of the buffer that was cleaned out to DRAM beforehand.
>>> >>
>>> >> I've tried both with and without [1] applied. I also tried removing
>>> >> the call to dma_unmap_single, but to no effect.
>>> >
>>> > Actually, I wasn't updating my device tree like I thought...
>>> >
>>> > Turns out I2C works only *without* this patch.
>>> >
>>> > So maybe the eDMA is not coherent?
>>>
>>> It seems like this might be the case. From the reference manual:
>>>
>>> > All transactions from eDMA are tagged as snoop configuration if the
>>> > SCFG_SNPCNFGCR[eDMASNP] bit is set. Refer Snoop Configuration
>>> Register
>>> > (SCFG_SNPCNFGCR) for details.
>>>
>>> But there is no such bit in this register on the LS1046A. On the LS1043A, this
>>> bit does exist, but on the LS1046A it is reserved. I read the register, and
>>> found the bit was 0. Perhaps eDMA was intended to be coherent, but there
>>> was a hardware bug?
>>
>> Thanks for the findings. I don't know the reason why this bit is reserved on LS1046a either. It should have a similar design as LS1043a.
>
> Funnily enough, this bit is not set for the LS1043A either [1]. So maybe
> this is a U-Boot bug? I'll test this tomorrow.
OK, this looks like it fixes things. I'll submit a patch to U-Boot. But
this bit should really be documented in the LS1046A manual as well.
--Sean
> [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/cpu/armv8/fsl-l…
>
>>>
>>> If this is the case, then I think dma-coherent should be left out of the top-
>>> level /soc node. Instead, the qdma, sata, usb, and emmc nodes should have
>>> dma-coherent added.
>>>
>>> Li/Laurentiu, what are your thoughts?
>>
>> Then looks like it is not correct to say all devices on the bus is coherent. But as we have the new "dma-noncoherent" property now and most of the devices are actually coherent, we probably can keep the bus as dma-coherent and mark exceptions with dma-noncoherent?
>
> Neat, I didn't know about that.
>
> Yes, that sounds good. For the moment, only i2c0 uses DMA, so we will
> only need it there. At some point, someone should send a patch enabling
> it for the rest of the I2C devices, as well as the LPUARTs and SPIs.
Hello,
This series moves all drivers to a dynamic dma-buf locking specification.
From now on all dma-buf importers are made responsible for holding
dma-buf's reservation lock around all operations performed over dma-bufs
in accordance to the locking specification. This allows us to utilize
reservation lock more broadly around kernel without fearing of a potential
deadlocks.
This patchset passes all i915 selftests. It was also tested using VirtIO,
Panfrost, Lima, Tegra, udmabuf, AMDGPU and Nouveau drivers. I tested cases
of display+GPU, display+V4L and GPU+V4L dma-buf sharing (where appropriate),
which covers majority of kernel drivers since rest of the drivers share
same or similar code paths.
Changelog:
v5: - Added acks and r-bs that were given to v4.
- Changed i915 preparation patch like was suggested by Michael Ruhl.
The scope of reservation locking is smaller now.
v4: - Added dma_buf_mmap() to the "locking convention" documentation,
which was missed by accident in v3.
- Added acks from Christian König, Tomasz Figa and Hans Verkuil that
they gave to couple v3 patches.
- Dropped the "_unlocked" postfix from function names that don't have
the locked variant, as was requested by Christian König.
- Factored out the per-driver preparations into separate patches
to ease reviewing of the changes, which is now doable without the
global dma-buf functions renaming.
- Factored out the dynamic locking convention enforcements into separate
patches which add the final dma_resv_assert_held(dmabuf->resv) to the
dma-buf API functions.
v3: - Factored out dma_buf_mmap_unlocked() and attachment functions
into aseparate patches, like was suggested by Christian König.
- Corrected and factored out dma-buf locking documentation into
a separate patch, like was suggested by Christian König.
- Intel driver dropped the reservation locking fews days ago from
its BO-release code path, but we need that locking for the imported
GEMs because in the end that code path unmaps the imported GEM.
So I added back the locking needed by the imported GEMs, updating
the "dma-buf attachment locking specification" patch appropriately.
- Tested Nouveau+Intel dma-buf import/export combo.
- Tested udmabuf import to i915/Nouveau/AMDGPU.
- Fixed few places in Etnaviv, Panfrost and Lima drivers that I missed
to switch to locked dma-buf vmapping in the drm/gem: Take reservation
lock for vmap/vunmap operations" patch. In a result invalidated the
Christian's r-b that he gave to v2.
- Added locked dma-buf vmap/vunmap functions that are needed for fixing
vmappping of Etnaviv, Panfrost and Lima drivers mentioned above.
I actually had this change stashed for the drm-shmem shrinker patchset,
but then realized that it's already needed by the dma-buf patches.
Also improved my tests to better cover these code paths.
v2: - Changed locking specification to avoid problems with a cross-driver
ww locking, like was suggested by Christian König. Now the attach/detach
callbacks are invoked without the held lock and exporter should take the
lock.
- Added "locking convention" documentation that explains which dma-buf
functions and callbacks are locked/unlocked for importers and exporters,
which was requested by Christian König.
- Added ack from Tomasz Figa to the V4L patches that he gave to v1.
Dmitry Osipenko (21):
dma-buf: Add unlocked variant of vmapping functions
dma-buf: Add unlocked variant of attachment-mapping functions
drm/gem: Take reservation lock for vmap/vunmap operations
drm/prime: Prepare to dynamic dma-buf locking specification
drm/armada: Prepare to dynamic dma-buf locking specification
drm/i915: Prepare to dynamic dma-buf locking specification
drm/omapdrm: Prepare to dynamic dma-buf locking specification
drm/tegra: Prepare to dynamic dma-buf locking specification
drm/etnaviv: Prepare to dynamic dma-buf locking specification
RDMA/umem: Prepare to dynamic dma-buf locking specification
misc: fastrpc: Prepare to dynamic dma-buf locking specification
xen/gntdev: Prepare to dynamic dma-buf locking specification
media: videobuf2: Prepare to dynamic dma-buf locking specification
media: tegra-vde: Prepare to dynamic dma-buf locking specification
dma-buf: Move dma_buf_vmap() to dynamic locking specification
dma-buf: Move dma_buf_attach() to dynamic locking specification
dma-buf: Move dma_buf_map_attachment() to dynamic locking
specification
dma-buf: Move dma_buf_mmap() to dynamic locking specification
dma-buf: Document dynamic locking convention
media: videobuf2: Stop using internal dma-buf lock
dma-buf: Remove obsoleted internal lock
Documentation/driver-api/dma-buf.rst | 6 +
drivers/dma-buf/dma-buf.c | 211 +++++++++++++++---
drivers/gpu/drm/armada/armada_gem.c | 8 +-
drivers/gpu/drm/drm_client.c | 4 +-
drivers/gpu/drm/drm_gem.c | 24 ++
drivers/gpu/drm/drm_gem_dma_helper.c | 6 +-
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +-
drivers/gpu/drm/drm_gem_ttm_helper.c | 9 +-
drivers/gpu/drm/drm_prime.c | 6 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 14 ++
.../drm/i915/gem/selftests/i915_gem_dmabuf.c | 16 +-
drivers/gpu/drm/lima/lima_sched.c | 4 +-
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_dump.c | 4 +-
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 6 +-
drivers/gpu/drm/qxl/qxl_object.c | 17 +-
drivers/gpu/drm/qxl/qxl_prime.c | 4 +-
drivers/gpu/drm/tegra/gem.c | 17 +-
drivers/infiniband/core/umem_dmabuf.c | 7 +-
.../common/videobuf2/videobuf2-dma-contig.c | 22 +-
.../media/common/videobuf2/videobuf2-dma-sg.c | 19 +-
.../common/videobuf2/videobuf2-vmalloc.c | 17 +-
.../platform/nvidia/tegra-vde/dmabuf-cache.c | 6 +-
drivers/misc/fastrpc.c | 6 +-
drivers/xen/gntdev-dmabuf.c | 8 +-
include/drm/drm_gem.h | 3 +
include/linux/dma-buf.h | 17 +-
29 files changed, 320 insertions(+), 155 deletions(-)
--
2.37.3
Thanks for pointing this out! It's indeed quite a bug.
Going to send a patch ASAP.
Regards,
Christian.
Am 20.09.22 um 13:47 schrieb Sudip Mukherjee:
> Hi All,
>
> The builds of arm64 allmodconfig with clang failed to build next-20220920
> with the error:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:1190:3: error: variable 'r' is uninitialized when used here [-Werror,-Wuninitialized]
> r |= !amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> ^
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c:1171:7: note: initialize the variable 'r' to silence this warning
> int r;
> ^
> = 0
> 1 error generated.
>
>
> git bisect pointed to c2b08e7a6d27 ("drm/amdgpu: move entity selection and job init earlier during CS")
>
> I will be happy to test any patch or provide any extra log if needed.
>
>
Fence signaling must be enabled to make sure that
the dma_fence_is_signaled() function ever returns true.
Since drivers and implementations sometimes mess this up,
this ensures correct behaviour when DEBUG_WW_MUTEX_SLOWPATH
is used during debugging.
This should make any implementation bugs resulting in not
signaled fences much more obvious.
Arvind Yadav (6):
[PATCH v4 1/6] dma-buf: Remove the signaled bit status check
[PATCH v4 2/6] dma-buf: set signaling bit for the stub fence
[PATCH v4 3/6] dma-buf: Enable signaling on fence for selftests
[PATCH v4 4/6] dma-buf: dma_fence_wait must enable signaling
[PATCH v4 5/6] drm/sched: Use parent fence instead of finished
[PATCH v4 6/6] dma-buf: Check status of enable-signaling bit on debug
drivers/dma-buf/Kconfig | 7 +++++++
drivers/dma-buf/dma-fence.c | 16 ++++++++++------
drivers/dma-buf/st-dma-fence-chain.c | 4 ++++
drivers/dma-buf/st-dma-fence-unwrap.c | 22 ++++++++++++++++++++++
drivers/dma-buf/st-dma-fence.c | 16 ++++++++++++++++
drivers/dma-buf/st-dma-resv.c | 10 ++++++++++
drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
include/linux/dma-fence.h | 5 +++++
8 files changed, 76 insertions(+), 8 deletions(-)
--
2.25.1