Re: [PATCH v2 2/3] arm64: dts: qcom: sc7280: Move video-firmware to chrome-common
On 11/22/2023 7:50 PM, Luca Weiss wrote:
> On Wed Nov 22, 2023 at 2:17 PM CET, Vikash Garodia wrote:
>>
>> On 10/2/2023 7:50 PM, Luca Weiss wrote:
>>> If the video-firmware node is present, the venus driver assumes we're on
>>> a system that doesn't use TZ for starting venus, like on ChromeOS
>>> devices.
>>>
>>> Move the video-firmware node to chrome-common.dtsi so we can use venus
>>> on a non-ChromeOS devices.
>>>
>>> At the same time also disable the venus node by default in the dtsi,
>>> like it's done on other SoCs.
>>>
>>> Reviewed-by: Bryan O'Donoghue
>>> Signed-off-by: Luca Weiss
>>> ---
>>> arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi | 8
>>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 6 ++
>>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>> b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>> index 5d462ae14ba1..cd491e4d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
>>> @@ -104,6 +104,14 @@ &scm {
>>> dma-coherent;
>>> };
>>>
>>> +&venus {
>>> + status = "okay";
>>> +
>>> + video-firmware {
>>> + iommus = <&apps_smmu 0x21a2 0x0>;
>>> + };
>>> +};
>>> +
>>> &watchdog {
>>> status = "okay";
>>> };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 66f1eb83cca7..fa53f54d4675 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -3740,6 +3740,8 @@ venus: video-codec@aa0 {
>>> <&apps_smmu 0x2184 0x20>;
0x2184 is a secure SID. I think qcm6490-fairphone-fp5.dts needs to override the
iommus property as well to retain only the non secure SID i.e 0x2180 ? I am
seeing below crash
Call trace:
[ 47.663593] qcom_smmu_write_s2cr+0x64/0xa4
[ 47.663616] arm_smmu_attach_dev+0x120/0x284
[ 47.663647] __iommu_attach_device+0x24/0xf8
[ 47.676845] __iommu_device_set_domain+0x70/0xd0
[ 47.681632] __iommu_group_set_domain_internal+0x60/0x1b4
[ 47.687218] iommu_setup_default_domain+0x358/0x418
[ 47.692258] __iommu_probe_device+0x3e4/0x404
Could you please reconfirm if Video SID 0x2184 (and mask) is allowed by the
qcm6490-fairphone-fp5 hardware having TZ ?
>>> memory-region = <&video_mem>;
>>>
>>> + status = "disabled";
>>> +
>>> video-decoder {
>>> compatible = "venus-decoder";
>>> };
>>> @@ -3748,10 +3750,6 @@ video-encoder {
>>> compatible = "venus-encoder";
>>> };
>>>
>>> - video-firmware {
>>> - iommus = <&apps_smmu 0x21a2 0x0>;
>>> - };
>>> -
>>> venus_opp_table: opp-table {
>>> compatible = "operating-points-v2";
>>>
>>>
>> Changes look good. Is this tested on SC7280 ?
>
> Hi Vikash,
>
> I didn't test it myself on sc7280 (just qcm6490-fp5) but dtx_diff
> reports no differences except for status = okay property being added, so
> there should be no change on those boards. See below.
>
> Regards
> Luca
I tested on SC7280 (herobrine) and all good.
Regards,
Vikash
[PATCH] rethook: Use __rcu pointer for rethook::handler
From: Masami Hiramatsu (Google)
Since the rethook::handler is an RCU-maganged pointer so that it will
notice readers the rethook is stopped (unregistered) or not, it should
be an __rcu pointer and use appropriate functions to be accessed. This
will use appropriate memory barrier when accessing it. OTOH, rethook::data
is never changed, so we don't need to check it in get_kretprobe().
Fixes: 54ecbe6f1ed5 ("rethook: Add a generic return hook")
Signed-off-by: Masami Hiramatsu (Google)
---
include/linux/kprobes.h |6 ++
include/linux/rethook.h |2 +-
kernel/trace/rethook.c | 21 -
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 64672bace560..0ff44d6633e3 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -197,10 +197,8 @@ extern int arch_trampoline_kprobe(struct kprobe *p);
#ifdef CONFIG_KRETPROBE_ON_RETHOOK
static nokprobe_inline struct kretprobe *get_kretprobe(struct
kretprobe_instance *ri)
{
- RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
- "Kretprobe is accessed from instance under preemptive context");
-
- return (struct kretprobe *)READ_ONCE(ri->node.rethook->data);
+ /* rethook::data is non-changed field, so that you can access it
freely. */
+ return (struct kretprobe *)ri->node.rethook->data;
}
static nokprobe_inline unsigned long get_kretprobe_retaddr(struct
kretprobe_instance *ri)
{
diff --git a/include/linux/rethook.h b/include/linux/rethook.h
index ce69b2b7bc35..164cd32c25cd 100644
--- a/include/linux/rethook.h
+++ b/include/linux/rethook.h
@@ -28,7 +28,7 @@ typedef void (*rethook_handler_t) (struct rethook_node *,
void *, unsigned long,
*/
struct rethook {
void*data;
- rethook_handler_t handler;
+ rethook_handler_t __rcu handler;
struct objpool_head pool;
struct rcu_head rcu;
};
diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c
index 6fd7d4ecbbc6..77f0e987bbff 100644
--- a/kernel/trace/rethook.c
+++ b/kernel/trace/rethook.c
@@ -48,7 +48,7 @@ static void rethook_free_rcu(struct rcu_head *head)
*/
void rethook_stop(struct rethook *rh)
{
- WRITE_ONCE(rh->handler, NULL);
+ rcu_assign_pointer(rh->handler, NULL);
}
/**
@@ -63,7 +63,7 @@ void rethook_stop(struct rethook *rh)
*/
void rethook_free(struct rethook *rh)
{
- WRITE_ONCE(rh->handler, NULL);
+ rcu_assign_pointer(rh->handler, NULL);
call_rcu(&rh->rcu, rethook_free_rcu);
}
@@ -107,7 +107,7 @@ struct rethook *rethook_alloc(void *data, rethook_handler_t
handler,
return ERR_PTR(-ENOMEM);
rh->data = data;
- rh->handler = handler;
+ rcu_assign_pointer(rh->handler, handler);
/* initialize the objpool for rethook nodes */
if (objpool_init(&rh->pool, num, size, GFP_KERNEL, rh,
@@ -135,9 +135,12 @@ static void free_rethook_node_rcu(struct rcu_head *head)
*/
void rethook_recycle(struct rethook_node *node)
{
- lockdep_assert_preemption_disabled();
+ rethook_handler_t handler;
+
+ handler = rcu_dereference_check(node->rethook->handler,
+ rcu_read_lock_any_held());
- if (likely(READ_ONCE(node->rethook->handler)))
+ if (likely(handler))
objpool_push(node, &node->rethook->pool);
else
call_rcu(&node->rcu, free_rethook_node_rcu);
@@ -153,10 +156,9 @@ NOKPROBE_SYMBOL(rethook_recycle);
*/
struct rethook_node *rethook_try_get(struct rethook *rh)
{
- rethook_handler_t handler = READ_ONCE(rh->handler);
-
- lockdep_assert_preemption_disabled();
+ rethook_handler_t handler;
+ handler = rcu_dereference_check(rh->handler, rcu_read_lock_any_held());
/* Check whether @rh is going to be freed. */
if (unlikely(!handler))
return NULL;
@@ -300,7 +302,8 @@ unsigned long rethook_trampoline_handler(struct pt_regs
*regs,
rhn = container_of(first, struct rethook_node, llist);
if (WARN_ON_ONCE(rhn->frame != frame))
break;
- handler = READ_ONCE(rhn->rethook->handler);
+ handler = rcu_dereference_check(rhn->rethook->handler,
+ rcu_read_lock_any_held());
if (handler)
handler(rhn, rhn->rethook->data,
correct_ret_addr, regs);
Re: [PATCH v7 3/4] remoteproc: zynqmp: add pm domains support
On Wed, Nov 22, 2023 at 03:00:36PM -0600, Tanmay Shah wrote:
> Hi Mathieu,
>
> Please find my comments below.
>
> On 11/21/23 4:59 PM, Mathieu Poirier wrote:
> > Hi,
> >
> > On Fri, Nov 17, 2023 at 09:42:37AM -0800, Tanmay Shah wrote:
> > > Use TCM pm domains extracted from device-tree
> > > to power on/off TCM using general pm domain framework.
> > >
> > > Signed-off-by: Tanmay Shah
> > > ---
> > >
> > > Changes in v7:
> > > - %s/pm_dev1/pm_dev_core0/r
> > > - %s/pm_dev_link1/pm_dev_core0_link/r
> > > - %s/pm_dev2/pm_dev_core1/r
> > > - %s/pm_dev_link2/pm_dev_core1_link/r
> > > - remove pm_domain_id check to move next patch
> > > - add comment about how 1st entry in pm domain list is used
> > > - fix loop when jump to fail_add_pm_domains loop
> > >
> > > drivers/remoteproc/xlnx_r5_remoteproc.c | 215 +++-
> > > 1 file changed, 212 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > index 4395edea9a64..22bccc5075a0 100644
> > > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > @@ -16,6 +16,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include "remoteproc_internal.h"
> > >
> > > @@ -102,6 +103,12 @@ static const struct mem_bank_data
> > > zynqmp_tcm_banks_lockstep[] = {
> > > * @rproc: rproc handle
> > > * @pm_domain_id: RPU CPU power domain id
> > > * @ipi: pointer to mailbox information
> > > + * @num_pm_dev: number of tcm pm domain devices for this core
> > > + * @pm_dev_core0: pm domain virtual devices for power domain framework
> > > + * @pm_dev_core0_link: pm domain device links after registration
> > > + * @pm_dev_core1: used only in lockstep mode. second core's pm domain
> > > virtual devices
> > > + * @pm_dev_core1_link: used only in lockstep mode. second core's pm
> > > device links after
> > > + * registration
> > > */
> > > struct zynqmp_r5_core {
> > > struct device *dev;
> > > @@ -111,6 +118,11 @@ struct zynqmp_r5_core {
> > > struct rproc *rproc;
> > > u32 pm_domain_id;
> > > struct mbox_info *ipi;
> > > + int num_pm_dev;
> > > + struct device **pm_dev_core0;
> > > + struct device_link **pm_dev_core0_link;
> > > + struct device **pm_dev_core1;
> > > + struct device_link **pm_dev_core1_link;
> > > };
> > >
> > > /**
> > > @@ -651,7 +663,8 @@ static int add_tcm_carveout_lockstep_mode(struct
> > > rproc *rproc)
> > >ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > >ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > > if (ret < 0) {
> > > - dev_err(dev, "failed to turn on TCM 0x%x",
> > > pm_domain_id);
> > > + dev_err(dev, "failed to turn on TCM 0x%x",
> > > + pm_domain_id);
> >
> > Spurious change, you should have caught that.
>
> Ack, need to observe changes more closely before sending them.
>
> >
> > > goto release_tcm_lockstep;
> > > }
> > >
> > > @@ -758,6 +771,189 @@ static int zynqmp_r5_parse_fw(struct rproc *rproc,
> > > const struct firmware *fw)
> > > return ret;
> > > }
> > >
> > > +static void zynqmp_r5_remove_pm_domains(struct rproc *rproc)
> > > +{
> > > + struct zynqmp_r5_core *r5_core = rproc->priv;
> > > + struct device *dev = r5_core->dev;
> > > + struct zynqmp_r5_cluster *cluster;
> > > + int i;
> > > +
> > > + cluster = platform_get_drvdata(to_platform_device(dev->parent));
> > > +
> > > + for (i = 1; i < r5_core->num_pm_dev; i++) {
> > > + device_link_del(r5_core->pm_dev_core0_link[i]);
> > > + dev_pm_domain_detach(r5_core->pm_dev_core0[i], false);
> > > + }
> > > +
> > > + kfree(r5_core->pm_dev_core0);
> > > + r5_core->pm_dev_core0 = NULL;
> > > + kfree(r5_core->pm_dev_core0_link);
> > > + r5_core->pm_dev_core0_link = NULL;
> > > +
> > > + if (cluster->mode == SPLIT_MODE) {
> > > + r5_core->num_pm_dev = 0;
> > > + return;
> > > + }
> > > +
> > > + for (i = 1; i < r5_core->num_pm_dev; i++) {
> > > + device_link_del(r5_core->pm_dev_core1_link[i]);
> > > + dev_pm_domain_detach(r5_core->pm_dev_core1[i], false);
> > > + }
> > > +
> > > + kfree(r5_core->pm_dev_core1);
> > > + r5_core->pm_dev_core1 = NULL;
> > > + kfree(r5_core->pm_dev_core1_link);
> > > + r5_core->pm_dev_core1_link = NULL;
> > > + r5_core->num_pm_dev = 0;
> > > +}
> > > +
> > > +static int zynqmp_r5_add_pm_domains(struct rproc *rproc)
> > > +{
> > > + struct zynqmp_r5_core *r5_core = rproc->priv;
> > > + struct device *dev = r5_core->dev, *dev2;
> > > + struct zynqmp_r5_cluster *cluster;
> > > + struct platform_device *pdev;
> > > + struct device_node *np;
> > > + int i, j, num_pm_dev, ret;
> > > +
> > > + cluster = dev_get_drvdata(dev->parent);
> > > +
> > > + /* get number of power-domains */
> > > + num_pm_dev = of_count_phandle_with_args(r5_core->n
Re: selftests: ftrace: WARNING: __list_del_entry_valid_or_report (lib/list_debug.c:62 (discriminator 1))
On Wed, Nov 22, 2023 at 10:12:51AM -0500, Steven Rostedt wrote: > On Wed, 22 Nov 2023 19:49:43 +0530 > Naresh Kamboju wrote: > > > Hi Steven, > > > > > > > > On Tue, 21 Nov 2023 at 02:06, Steven Rostedt wrote: > > > > > > On Thu, 16 Nov 2023 18:00:16 +0530 > > > Naresh Kamboju wrote: > > [ 282.726999] Unexpected kernel BRK exception at EL1 > > What's a "BRK exception"? That's triggered by a BRK instruction (software breakpoint), we use it like UD2 on x86. > > [ 282.731840] Internal error: BRK handler: f20003e8 [#1] PREEMPT > > SMP That lump of hex here means "this was triggered by a BRK #1000" instruction. That immediate (0x3e8 / 1000) is what GCC/Clang use for __builtin_trap(), which is generated for a bunch of reasons, usually a sanitizer like UBSAN, or options to trap on runtime issues. Naresh, where can I find the config used for this run? I can try to reproduce this and investigate. Thanks, Mark. > > [ 282.738752] Modules linked in: ftrace_direct ftrace_direct_too > > hdlcd tda998x onboard_usb_hub drm_dma_helper cec crct10dif_ce > > drm_kms_helper fuse drm backlight dm_mod ip_tables x_tables [last > > unloaded: ftrace_direct] > > [ 282.758152] CPU: 0 PID: 987 Comm: rmmod Not tainted 6.6.2-rc1 #1 > > So this happened on removing a module? Unloading ftrace_direct? > > This could be a direct trampoline bug. > > But it doesn't look to be related to eventfs, so I'm going with my other > patches. > > Thanks, > > -- Steve > > > > [ 282.764191] Hardware name: ARM Juno development board (r2) (DT) > > [ 282.770138] pstate: a0c5 (NzCv daIF -PAN -UAO -TCO -DIT -SSBS > > BTYPE=--) > > [ 282.777133] pc : ring_buffer_lock_reserve > > (kernel/trace/ring_buffer.c:3304 (discriminator 1) > > kernel/trace/ring_buffer.c:3819 (discriminator 1)) > > [ 282.782230] lr : trace_function (kernel/trace/trace.c:992 > > kernel/trace/trace.c:3078) > > [ 282.786360] sp : 80008428ba20 > > [ 282.789695] x29: 80008428ba20 x28: 00082bb367c0 x27: > > > > [ 282.796898] x26: 800080371d84 x25: x24: > > 00097ed1e4b0 > > [ 282.804100] x23: 000800018400 x22: x21: > > 0018 > > [ 282.811302] x20: 000800018400 x19: 000800041400 x18: > > > > [ 282.818504] x17: x16: x15: > > 800081d2ec58 > > [ 282.825706] x14: x13: 80008428beb0 x12: > > 0255 > > [ 282.832908] x11: 800083343078 x10: 80008428bbc0 x9 : > > 8000803e3110 > > [ 282.840110] x8 : 80008428ba98 x7 : x6 : > > 0009 > > [ 282.847311] x5 : 0081 x4 : fffb x3 : > > 0001 > > [ 282.854513] x2 : x1 : x0 : > > > > [ 282.861715] Call trace: > > [ 282.864179] ring_buffer_lock_reserve > > (kernel/trace/ring_buffer.c:3304 (discriminator 1) > > kernel/trace/ring_buffer.c:3819 (discriminator 1)) > > [ 282.868920] trace_function (kernel/trace/trace.c:992 > > kernel/trace/trace.c:3078) > > [ 282.872702] tracer_hardirqs_off (kernel/trace/trace_irqsoff.c:279 > > kernel/trace/trace_irqsoff.c:397 kernel/trace/trace_irqsoff.c:619 > > kernel/trace/trace_irqsoff.c:616) > > [ 282.877009] trace_hardirqs_off.part.0 > > (kernel/trace/trace_preemptirq.c:78 (discriminator 1)) > > [ 282.881748] trace_hardirqs_off (kernel/trace/trace_preemptirq.c:94) > > [ 282.885791] obj_cgroup_charge (mm/memcontrol.c:3236 (discriminator > > 1) mm/memcontrol.c:3364 (discriminator 1)) > > [ 282.889918] kmem_cache_alloc (mm/slab.h:503 (discriminator 2) > > mm/slab.h:714 (discriminator 2) mm/slub.c:3460 (discriminator 2) > > mm/slub.c:3486 (discriminator 2) mm/slub.c:3493 (discriminator 2) > > mm/slub.c:3502 (discriminator 2)) > > [ 282.893873] __anon_vma_prepare (mm/rmap.c:197) > > [ 282.897999] __handle_mm_fault (mm/memory.c:4111 (discriminator 2) > > mm/memory.c:3670 (discriminator 2) mm/memory.c:4981 (discriminator 2) > > mm/memory.c:5122 (discriminator 2)) > > [ 282.902213] handle_mm_fault (mm/memory.c:5287) > > [ 282.906077] do_page_fault (arch/arm64/mm/fault.c:513 > > arch/arm64/mm/fault.c:626) > > [ 282.909856] do_translation_fault (arch/arm64/mm/fault.c:714) > > [ 282.914068] do_mem_abort (arch/arm64/mm/fault.c:846 (discriminator 1)) > > [ 282.917591] el0_da (arch/arm64/include/asm/daifflags.h:28 > > arch/arm64/kernel/entry-common.c:133 > > arch/arm64/kernel/entry-common.c:144 > > arch/arm64/kernel/entry-common.c:547) > > [ 282.920590] el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:700) > > [ 282.924894] el0t_64_sync (arch/arm64/kernel/entry.S:595) > > [ 282.928593] Code: 88037c22 35a3 17f8 94549eb9 (d4207d00) > > All code > > > >0: 88037c22 stxr w3, w2, [x1] > >4: 35a3 cbnz w3, 0xfff8 > >8: 17f8 b 0xffe8 > >c: 94549eb9 bl 0x1527af0 > > 10:* d4207d00 brk #0x3e8 <-- trapp
Re: [PATCH net v1] vsock/test: fix SEQPACKET message bounds test
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski : On Wed, 22 Nov 2023 00:16:42 +0300 you wrote: > Tune message length calculation to make this test work on machines > where 'getpagesize()' returns >32KB. Now maximum message length is not > hardcoded (on machines above it was smaller than 'getpagesize()' return > value, thus we get negative value and test fails), but calculated at > runtime and always bigger than 'getpagesize()' result. Reproduced on > aarch64 with 64KB page size. > > [...] Here is the summary with links: - [net,v1] vsock/test: fix SEQPACKET message bounds test https://git.kernel.org/netdev/net/c/f0863888f6cf You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH v5] eventfs: Remove eventfs_file and just use eventfs_inode
On Thu, Nov 23, 2023 at 10:23:49AM -0500, Steven Rostedt wrote:
> On Thu, 23 Nov 2023 12:25:48 +0100
> Heiko Carstens wrote:
>
> > So, if it helps (this still happens with Linus' master branch):
> >
> > create_dir_dentry() is called with a "struct eventfs_inode *ei" (second
> > parameter), which points to a data structure where "is_freed" is 1. Then it
> > looks like create_dir() returned "-EEXIST". And looking at the code this
> > combination then must lead to d_invalidate() incorrectly being called with
> > "-EEXIST" as dentry pointer.
>
> I haven't looked too much at the error codes, let me do that on Monday
> (it's currently Turkey weekend here in the US).
>
> But could you test this branch:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
> trace/core
>
> I have a bunch of fixes in that branch that may fix your issue. I just
> finished testing it and plan on pushing it to Linus before the next rc
> release.
This is not that easy to reproduce, however you branch contains commit
71cade82f2b5 ("eventfs: Do not invalidate dentry in create_file/dir_dentry()")
which removes the d_invalidate() call.
The crash I reported cannot happen anymore with that commit. I'll consider
this fixed, and report again if this (or something else) still causes
problems.
Thanks!
Re: [PATCH v5] eventfs: Remove eventfs_file and just use eventfs_inode
On Thu, 23 Nov 2023 12:25:48 +0100 Heiko Carstens wrote: > So, if it helps (this still happens with Linus' master branch): > > create_dir_dentry() is called with a "struct eventfs_inode *ei" (second > parameter), which points to a data structure where "is_freed" is 1. Then it > looks like create_dir() returned "-EEXIST". And looking at the code this > combination then must lead to d_invalidate() incorrectly being called with > "-EEXIST" as dentry pointer. I haven't looked too much at the error codes, let me do that on Monday (it's currently Turkey weekend here in the US). But could you test this branch: git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace/core I have a bunch of fixes in that branch that may fix your issue. I just finished testing it and plan on pushing it to Linus before the next rc release. Thanks! -- Steve > > Now, I have no idea how the code should work, but it is quite obvious that > something is broken :) > > Here the dump of the struct eventfs_inode that was passed to > create_file_dentry() when the crash happened:
[syzbot] Monthly trace report (Nov 2023)
Hello trace maintainers/developers, This is a 31-day syzbot report for the trace subsystem. All related reports/information can be found at: https://syzkaller.appspot.com/upstream/s/trace During the period, 3 new issues were detected and 0 were fixed. In total, 5 issues are still open and 29 have been fixed so far. Some of the still happening issues: Ref Crashes Repro Title <1> 26 Yes WARNING in blk_register_tracepoints https://syzkaller.appspot.com/bug?extid=c54ded83396afee31eb1 <2> 5 Yes WARNING in get_probe_ref https://syzkaller.appspot.com/bug?extid=8672dcb9d10011c0a160 <3> 1 Nopossible deadlock in sctp_err_lookup https://syzkaller.appspot.com/bug?extid=422ecd5adb35122711b7 --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkal...@googlegroups.com. To disable reminders for individual bugs, reply with the following command: #syz set no-reminders To change bug's subsystems, reply with: #syz set subsystems: new-subsystem You may send multiple commands in a single email message.
Re: [PATCH v5] eventfs: Remove eventfs_file and just use eventfs_inode
> On 23-Nov-2023, at 4:55 PM, Heiko Carstens wrote:
>
> !! External Email
>
> On Fri, Nov 17, 2023 at 03:38:29PM +0100, Heiko Carstens wrote:
>> On Fri, Nov 17, 2023 at 03:23:35PM +0100, Heiko Carstens wrote:
>>> I think this patch causes from time to time crashes when running ftrace
>>> selftests. In particular I guess there is a bug wrt error handling in this
>>> function (see below for call trace):
>>>
+static struct dentry *
+create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
+struct dentry *parent, const char *name, umode_t mode, void
*data,
+const struct file_operations *fops, bool lookup)
+{
>> ...
>>> Note that the compare and swap instruction within d_invalidate() generates
>>> a specification exception because it operates on an invalid address
>>> (0xffef), which happens to be -EEXIST. So my assumption is that
>>> create_dir_dentry() has incorrect error handling and passes -EEXIST instead
>>> of a valid dentry pointer to d_invalidate().
>>>
>>> But I leave it up to you to figure this out :)
>>
>> Ok, wrong function quoted of course. But the rest of my statement
>> should be correct.
>
> So, if it helps (this still happens with Linus' master branch):
>
> create_dir_dentry() is called with a "struct eventfs_inode *ei" (second
> parameter), which points to a data structure where "is_freed" is 1. Then it
> looks like create_dir() returned "-EEXIST". And looking at the code this
> combination then must lead to d_invalidate() incorrectly being called with
> "-EEXIST" as dentry pointer.
>
> Now, I have no idea how the code should work, but it is quite obvious that
> something is broken :)
>
> Here the dump of the struct eventfs_inode that was passed to
> create_file_dentry() when the crash happened:
>
> crash> struct eventfs_inode eada7680
> struct eventfs_inode {
> list = {
>next = 0x10f802da0,
>prev = 0x122
> },
> entries = 0x12c031328 ,
> name = 0x12b90bbac <__tpstrtab_xfs_alloc_vextent_exact_bno>
> "xfs_alloc_vextent_exact_bno",
> children = {
>next = 0xeada76a0,
>prev = 0xeada76a0
> },
> dentry = 0x0,
> d_parent = 0x107c75d40,
> d_children = 0xeada5700,
> entry_attrs = 0x0,
> attr = {
>mode = 0,
>uid = {
> val = 0
>},
>gid = {
> val = 0
>}
> },
> data = 0xeada6660,
> {
>llist = {
> next = 0xeada7668
>},
>rcu = {
> next = 0xeada7668,
> func = 0x12ad2a5b8
>}
> },
> is_freed = 1,
> nr_entries = 6
> }
Heiko, your analysis looks good to me. Seems -EEXIST is from:
https://elixir.bootlin.com/linux/v6.7-rc2/source/fs/tracefs/inode.c#L533
Steve, as per me error handling should be same for create_dir_dentry()
and create_file_dentry() or am I missing something.
-Ajay
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Thu, Nov 23, 2023 at 08:38:45PM +0900, Masahiro Yamada wrote: > On Thu, Nov 23, 2023 at 6:05 PM Greg KH wrote: > > > > On Wed, Nov 22, 2023 at 01:04:09PM -0800, Matthew Maurer wrote: > > > > So, even if you enable CONFIG_MODVERSIONS, > > > > nothing is checked for Rust. > > > > Genksyms computes a CRC from "int foo", and > > > > the module subsystem confirms it is a "int" > > > > variable. > > > > > > > > We know this check always succeeds. > > > > > > > > Why is this useful? > > > The reason this is immediately useful is that it allows us to have Rust > > > in use with a kernel where C modules are able to benefit from MODVERSIONS > > > checking. The check would effectively be a no-op for now, as you have > > > correctly > > > determined, but we could refine it to make it more restrictive later. > > > Since the > > > existing C approach errs on the side of "it could work" rather than "it > > > will > > > work", I thought being more permissive was the correct initial solution. > > > > But it's just providing "fake" information to the CRC checker, which > > means that the guarantee of a ABI check is not true at all. > > > > So the ask for the user of "ensure that the ABI checking is correct" is > > being circumvented here, and any change in the rust side can not be > > detected at all. > > > > The kernel is a "whole", either an option works for it, or it doesn't, > > and you are splitting that guarantee here by saying "modversions will > > only work for a portion of the kernel, not the whole thing" which is > > going to cause problems for when people expect it to actually work > > properly. > > > > So, I'd strongly recommend fixing this for the rust code if you wish to > > allow modversions to be enabled at all. > > > > > With regards to future directions that likely won't work for loosening it: > > > Unfortunately, the .rmeta format itself is not stable, so I wouldn't want > > > to > > > teach genksyms to open it up and split out the pieces for specific > > > functions. > > > Extending genksyms to parse Rust would also not solve the situation - > > > layouts are allowed to differ across compiler versions or even (in rare > > > cases) seemingly unrelated code changes. > > > > What do you mean by "layout" here? Yes, the crcs can be different > > across compiler versions and seemingly unrelated code changes (genksyms > > is VERY fragile) but that's ok, that's not what you are checking here. > > You want to know if the rust function signature changes or not from the > > last time you built the code, with the same compiler and options, that's > > all you are verifying. > > > > > Future directions that might work for loosening it: > > > * Generating crcs from debuginfo + compiler + flags > > > * Adding a feature to the rust compiler to dump this information. This > > > is likely to > > > get pushback because Rust's current stance is that there is no ability > > > to load > > > object code built against a different library. > > > > Why not parse the function signature like we do for C? > > > > > Would setting up Rust symbols so that they have a crc built out of .rmeta > > > be > > > sufficient for you to consider this useful? If not, can you help me > > > understand > > > what level of precision would be required? > > > > What exactly does .rmeta have to do with the function signature? That's > > all you care about here. > > > > > rmeta is generated per crate. > > CRC is computed per symbol. > > They have different granularity. > It is weird to refuse a module for incompatibility > of a symbol that it is not using at all. I agree, this should be on a per-symbol basis, so the Rust infrastructure in the kernel needs to be fixed up to support this properly, not just ignored like this patchset does. thanks, greg k-h
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Thu, Nov 23, 2023 at 6:05 PM Greg KH wrote: > > On Wed, Nov 22, 2023 at 01:04:09PM -0800, Matthew Maurer wrote: > > > So, even if you enable CONFIG_MODVERSIONS, > > > nothing is checked for Rust. > > > Genksyms computes a CRC from "int foo", and > > > the module subsystem confirms it is a "int" > > > variable. > > > > > > We know this check always succeeds. > > > > > > Why is this useful? > > The reason this is immediately useful is that it allows us to have Rust > > in use with a kernel where C modules are able to benefit from MODVERSIONS > > checking. The check would effectively be a no-op for now, as you have > > correctly > > determined, but we could refine it to make it more restrictive later. > > Since the > > existing C approach errs on the side of "it could work" rather than "it will > > work", I thought being more permissive was the correct initial solution. > > But it's just providing "fake" information to the CRC checker, which > means that the guarantee of a ABI check is not true at all. > > So the ask for the user of "ensure that the ABI checking is correct" is > being circumvented here, and any change in the rust side can not be > detected at all. > > The kernel is a "whole", either an option works for it, or it doesn't, > and you are splitting that guarantee here by saying "modversions will > only work for a portion of the kernel, not the whole thing" which is > going to cause problems for when people expect it to actually work > properly. > > So, I'd strongly recommend fixing this for the rust code if you wish to > allow modversions to be enabled at all. > > > With regards to future directions that likely won't work for loosening it: > > Unfortunately, the .rmeta format itself is not stable, so I wouldn't want to > > teach genksyms to open it up and split out the pieces for specific > > functions. > > Extending genksyms to parse Rust would also not solve the situation - > > layouts are allowed to differ across compiler versions or even (in rare > > cases) seemingly unrelated code changes. > > What do you mean by "layout" here? Yes, the crcs can be different > across compiler versions and seemingly unrelated code changes (genksyms > is VERY fragile) but that's ok, that's not what you are checking here. > You want to know if the rust function signature changes or not from the > last time you built the code, with the same compiler and options, that's > all you are verifying. > > > Future directions that might work for loosening it: > > * Generating crcs from debuginfo + compiler + flags > > * Adding a feature to the rust compiler to dump this information. This > > is likely to > > get pushback because Rust's current stance is that there is no ability to > > load > > object code built against a different library. > > Why not parse the function signature like we do for C? > > > Would setting up Rust symbols so that they have a crc built out of .rmeta be > > sufficient for you to consider this useful? If not, can you help me > > understand > > what level of precision would be required? > > What exactly does .rmeta have to do with the function signature? That's > all you care about here. rmeta is generated per crate. CRC is computed per symbol. They have different granularity. It is weird to refuse a module for incompatibility of a symbol that it is not using at all. > thanks, > > greg k-h -- Best Regards Masahiro Yamada
Re: [PATCH v5] eventfs: Remove eventfs_file and just use eventfs_inode
On Fri, Nov 17, 2023 at 03:38:29PM +0100, Heiko Carstens wrote:
> On Fri, Nov 17, 2023 at 03:23:35PM +0100, Heiko Carstens wrote:
> > I think this patch causes from time to time crashes when running ftrace
> > selftests. In particular I guess there is a bug wrt error handling in this
> > function (see below for call trace):
> >
> > > +static struct dentry *
> > > +create_file_dentry(struct eventfs_inode *ei, struct dentry **e_dentry,
> > > +struct dentry *parent, const char *name, umode_t mode, void
> > > *data,
> > > +const struct file_operations *fops, bool lookup)
> > > +{
> ...
> > Note that the compare and swap instruction within d_invalidate() generates
> > a specification exception because it operates on an invalid address
> > (0xffef), which happens to be -EEXIST. So my assumption is that
> > create_dir_dentry() has incorrect error handling and passes -EEXIST instead
> > of a valid dentry pointer to d_invalidate().
> >
> > But I leave it up to you to figure this out :)
>
> Ok, wrong function quoted of course. But the rest of my statement
> should be correct.
So, if it helps (this still happens with Linus' master branch):
create_dir_dentry() is called with a "struct eventfs_inode *ei" (second
parameter), which points to a data structure where "is_freed" is 1. Then it
looks like create_dir() returned "-EEXIST". And looking at the code this
combination then must lead to d_invalidate() incorrectly being called with
"-EEXIST" as dentry pointer.
Now, I have no idea how the code should work, but it is quite obvious that
something is broken :)
Here the dump of the struct eventfs_inode that was passed to
create_file_dentry() when the crash happened:
crash> struct eventfs_inode eada7680
struct eventfs_inode {
list = {
next = 0x10f802da0,
prev = 0x122
},
entries = 0x12c031328 ,
name = 0x12b90bbac <__tpstrtab_xfs_alloc_vextent_exact_bno>
"xfs_alloc_vextent_exact_bno",
children = {
next = 0xeada76a0,
prev = 0xeada76a0
},
dentry = 0x0,
d_parent = 0x107c75d40,
d_children = 0xeada5700,
entry_attrs = 0x0,
attr = {
mode = 0,
uid = {
val = 0
},
gid = {
val = 0
}
},
data = 0xeada6660,
{
llist = {
next = 0xeada7668
},
rcu = {
next = 0xeada7668,
func = 0x12ad2a5b8
}
},
is_freed = 1,
nr_entries = 6
}
[PATCH] net: make config lines follow common pattern
The Kconfig parser is quite relaxed when parsing config definition lines. However, there are just a few config definition lines that do not follow the common regular expression 'config [0-9A-Z]', i.e., there are only a few cases where config is not followed by exactly one whitespace. To simplify life for kernel developers that use basic regular expressions to find and extract kernel configs, make all config lines follow this common pattern. No functional change, just helpful stylistic clean-up. Signed-off-by: Lukas Bulwahn --- drivers/net/ethernet/cavium/Kconfig | 4 +-- net/caif/Kconfig| 2 +- net/netfilter/ipvs/Kconfig | 54 ++--- net/unix/Kconfig| 2 +- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/drivers/net/ethernet/cavium/Kconfig b/drivers/net/ethernet/cavium/Kconfig index ca742cc146d7..fe4203b1bc5f 100644 --- a/drivers/net/ethernet/cavium/Kconfig +++ b/drivers/net/ethernet/cavium/Kconfig @@ -32,7 +32,7 @@ config THUNDER_NIC_VF help This driver supports Thunder's NIC virtual function -config THUNDER_NIC_BGX +config THUNDER_NIC_BGX tristate "Thunder MAC interface driver (BGX)" depends on 64BIT && PCI select PHYLIB @@ -42,7 +42,7 @@ configTHUNDER_NIC_BGX This driver supports programming and controlling of MAC interface from NIC physical function driver. -config THUNDER_NIC_RGX +config THUNDER_NIC_RGX tristate "Thunder MAC interface driver (RGX)" depends on 64BIT && PCI select PHYLIB diff --git a/net/caif/Kconfig b/net/caif/Kconfig index 87205251cc25..1eb271125eb0 100644 --- a/net/caif/Kconfig +++ b/net/caif/Kconfig @@ -22,7 +22,7 @@ menuconfig CAIF See Documentation/networking/caif for a further explanation on how to use and configure CAIF. -config CAIF_DEBUG +config CAIF_DEBUG bool "Enable Debug" depends on CAIF default n diff --git a/net/netfilter/ipvs/Kconfig b/net/netfilter/ipvs/Kconfig index 2a3017b9c001..db6df2ca129d 100644 --- a/net/netfilter/ipvs/Kconfig +++ b/net/netfilter/ipvs/Kconfig @@ -26,7 +26,7 @@ menuconfig IP_VS if IP_VS -config IP_VS_IPV6 +config IP_VS_IPV6 bool "IPv6 support for IPVS" depends on IPV6 = y || IP_VS = IPV6 select NF_DEFRAG_IPV6 @@ -35,14 +35,14 @@ config IP_VS_IPV6 Say Y if unsure. -config IP_VS_DEBUG +config IP_VS_DEBUG bool "IP virtual server debugging" help Say Y here if you want to get additional messages useful in debugging the IP virtual server code. You can change the debug level in /proc/sys/net/ipv4/vs/debug_level -config IP_VS_TAB_BITS +config IP_VS_TAB_BITS int "IPVS connection table size (the Nth power of 2)" range 8 20 if !64BIT range 8 27 if 64BIT @@ -76,34 +76,34 @@ config IP_VS_TAB_BITS comment "IPVS transport protocol load balancing support" -config IP_VS_PROTO_TCP +config IP_VS_PROTO_TCP bool "TCP load balancing support" help This option enables support for load balancing TCP transport protocol. Say Y if unsure. -config IP_VS_PROTO_UDP +config IP_VS_PROTO_UDP bool "UDP load balancing support" help This option enables support for load balancing UDP transport protocol. Say Y if unsure. -config IP_VS_PROTO_AH_ESP +config IP_VS_PROTO_AH_ESP def_bool IP_VS_PROTO_ESP || IP_VS_PROTO_AH -config IP_VS_PROTO_ESP +config IP_VS_PROTO_ESP bool "ESP load balancing support" help This option enables support for load balancing ESP (Encapsulation Security Payload) transport protocol. Say Y if unsure. -config IP_VS_PROTO_AH +config IP_VS_PROTO_AH bool "AH load balancing support" help This option enables support for load balancing AH (Authentication Header) transport protocol. Say Y if unsure. -config IP_VS_PROTO_SCTP +config IP_VS_PROTO_SCTP bool "SCTP load balancing support" select LIBCRC32C help @@ -112,7 +112,7 @@ config IP_VS_PROTO_SCTP comment "IPVS scheduler" -config IP_VS_RR +config IP_VS_RR tristate "round-robin scheduling" help The robin-robin scheduling algorithm simply directs network @@ -121,7 +121,7 @@ config IP_VS_RR If you want to compile it in kernel, say Y. To compile it as a module, choose M here. If unsure, say N. -config IP_VS_WRR +config IP_VS_WRR tristate "weighted round-robin scheduling" help The weighted robin-robin scheduling algorithm directs network @@ -134,7 +134,7 @@ config IP_VS_WRR If you want to compile it in kernel, say Y. To compile it as a module, choose M here. If unsure, say N. -config IP_VS_LC +config IP_VS_LC tristate "least-connection scheduling" help
Re: [PATCH v2 0/5] MODVERSIONS + RUST Redux
On Wed, Nov 22, 2023 at 01:04:09PM -0800, Matthew Maurer wrote: > > So, even if you enable CONFIG_MODVERSIONS, > > nothing is checked for Rust. > > Genksyms computes a CRC from "int foo", and > > the module subsystem confirms it is a "int" > > variable. > > > > We know this check always succeeds. > > > > Why is this useful? > The reason this is immediately useful is that it allows us to have Rust > in use with a kernel where C modules are able to benefit from MODVERSIONS > checking. The check would effectively be a no-op for now, as you have > correctly > determined, but we could refine it to make it more restrictive later. > Since the > existing C approach errs on the side of "it could work" rather than "it will > work", I thought being more permissive was the correct initial solution. But it's just providing "fake" information to the CRC checker, which means that the guarantee of a ABI check is not true at all. So the ask for the user of "ensure that the ABI checking is correct" is being circumvented here, and any change in the rust side can not be detected at all. The kernel is a "whole", either an option works for it, or it doesn't, and you are splitting that guarantee here by saying "modversions will only work for a portion of the kernel, not the whole thing" which is going to cause problems for when people expect it to actually work properly. So, I'd strongly recommend fixing this for the rust code if you wish to allow modversions to be enabled at all. > With regards to future directions that likely won't work for loosening it: > Unfortunately, the .rmeta format itself is not stable, so I wouldn't want to > teach genksyms to open it up and split out the pieces for specific functions. > Extending genksyms to parse Rust would also not solve the situation - > layouts are allowed to differ across compiler versions or even (in rare > cases) seemingly unrelated code changes. What do you mean by "layout" here? Yes, the crcs can be different across compiler versions and seemingly unrelated code changes (genksyms is VERY fragile) but that's ok, that's not what you are checking here. You want to know if the rust function signature changes or not from the last time you built the code, with the same compiler and options, that's all you are verifying. > Future directions that might work for loosening it: > * Generating crcs from debuginfo + compiler + flags > * Adding a feature to the rust compiler to dump this information. This > is likely to > get pushback because Rust's current stance is that there is no ability to > load > object code built against a different library. Why not parse the function signature like we do for C? > Would setting up Rust symbols so that they have a crc built out of .rmeta be > sufficient for you to consider this useful? If not, can you help me understand > what level of precision would be required? What exactly does .rmeta have to do with the function signature? That's all you care about here. thanks, greg k-h
