| Message ID | 1375438577-30933-1-git-send-email-dborkman@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Daniel Borkmann <dborkman@redhat.com> Date: Fri, 2 Aug 2013 12:16:17 +0200 > Therefore, I strongly assume sizeof(*health->health) is being meant > to be passed as an argument. Interestingly, there are actually no > in-tree users of mlx5_[un]register_health_report_handler(), but some > debugging modules might want to know the correct size instead. I want these hooks and infrastructure removed immediately. If there are no in-tree users there is no reason for them to exist at all. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/03/2013 12:11 AM, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Fri, 2 Aug 2013 12:16:17 +0200 > >> Therefore, I strongly assume sizeof(*health->health) is being meant >> to be passed as an argument. Interestingly, there are actually no >> in-tree users of mlx5_[un]register_health_report_handler(), but some >> debugging modules might want to know the correct size instead. > > I want these hooks and infrastructure removed immediately. > > If there are no in-tree users there is no reason for them to > exist at all. Ok, I let Or handle that. Maybe he wants to add a user of it, instead. I also noticed that coverty scanner found a couple of other issues, e.g. outlen_write() in mlx5/core/cmd.c does a kzalloc() without doing sanity checks on the user-passed allocation size, e.g. it could even be a negative value passed to it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 3, 2013 at 1:43 AM, Daniel Borkmann <dborkman@redhat.com> wrote: > On 08/03/2013 12:11 AM, David Miller wrote: >> >> From: Daniel Borkmann <dborkman@redhat.com> >> Date: Fri, 2 Aug 2013 12:16:17 +0200 >> >>> Therefore, I strongly assume sizeof(*health->health) is being meant >>> to be passed as an argument. Interestingly, there are actually no >>> in-tree users of mlx5_[un]register_health_report_handler(), but some >>> debugging modules might want to know the correct size instead. >> >> >> I want these hooks and infrastructure removed immediately. >> >> If there are no in-tree users there is no reason for them to >> exist at all. > > > Ok, I let Or handle that. Maybe he wants to add a user of it, instead. > > I also noticed that coverty scanner found a couple of other issues, > e.g. outlen_write() in mlx5/core/cmd.c does a kzalloc() without > doing sanity checks on the user-passed allocation size, e.g. it > could even be a negative value passed to it. Hi Daniel, As listed in the kernel maintainers file, Eli Cohen (copied) is the upstream maintainer for the mlx5 driver, please make sure to send/copy him on patches/questions you have on mlx5. I will let Eli answer/handle your findings/questions. Or. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 02, 2013 at 03:11:32PM -0700, David Miller wrote: > From: Daniel Borkmann <dborkman@redhat.com> > Date: Fri, 2 Aug 2013 12:16:17 +0200 > > > Therefore, I strongly assume sizeof(*health->health) is being meant > > to be passed as an argument. Interestingly, there are actually no Daniel, you're fix is correct. I meant to pass the size of the struct and not the size of the pointer. > > in-tree users of mlx5_[un]register_health_report_handler(), but some > > debugging modules might want to know the correct size instead. > > I want these hooks and infrastructure removed immediately. > > If there are no in-tree users there is no reason for them to > exist at all. > Hi Dave, the intention here was to allow other module, if they so wish, to get notified of any problems detected at the device. I understand that you don't like this but is there another way to handle this requirement which will be accepted? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eli Cohen <eli@dev.mellanox.co.il> Date: Mon, 5 Aug 2013 09:33:34 +0300 > the intention here was to allow other module, if they so wish, to get > notified of any problems detected at the device. I understand that you > don't like this but is there another way to handle this requirement > which will be accepted? Which modules? I do not see them. Please remove this unused facility, now. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/health.c b/drivers/net/ethernet/mellanox/mlx5/core/health.c index 748f10a..3592e43 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/health.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/health.c @@ -101,7 +101,7 @@ static void health_care(struct work_struct *work) spin_lock_irq(&health_lock); if (reg_handler) reg_handler(dev->pdev, health->health, - sizeof(health->health)); + sizeof(*health->health)); list_del_init(&health->list); spin_unlock_irq(&health_lock);
In mlx5's function health_care() the callback handler reg_handler() is being called that checks the devices registers like: reg_handler(dev->pdev, health->health, sizeof(health->health)); health->health is a pointer to the member "struct health_buffer __iomem *health" of mlx5_core_health, where health buffer itself looks like: struct health_buffer { __be32 assert_var[5]; __be32 rsvd0[3]; __be32 assert_exit_ptr; __be32 assert_callra; __be32 rsvd1[2]; ... __be16 ext_sync; }; Therefore, I strongly assume sizeof(*health->health) is being meant to be passed as an argument. Interestingly, there are actually no in-tree users of mlx5_[un]register_health_report_handler(), but some debugging modules might want to know the correct size instead. Signed-off-by: Daniel Borkmann <dborkman@redhat.com> Cc: Or Gerlitz <ogerlitz@mellanox.com> --- drivers/net/ethernet/mellanox/mlx5/core/health.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)