| Message ID | 20241109-fix-oops-__smc_diag_dump-v1-1-1c55a3e54ad4@iiitd.ac.in (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove unused function parameter in __smc_diag_dump | expand |
Manas <devnull+manas18244.iiitd.ac.in@kernel.org> wrote: > The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. > There is only one instance of this function being called and its passed > with a NULL value in place of bc. > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > --- > The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. > There is only one instance of this function being called and its passed > with a NULL value in place of bc. > > Though, the compiler (gcc) optimizes it. Looking at the object dump of > vmlinux (via `objdump -D vmlinux`), a new function clone > (__smc_diag_dump.constprop.0) is added which removes this parameter from > calling convention altogether. > > ffffffff8a701770 <__smc_diag_dump.constprop.0>: > ffffffff8a701770: 41 57 push %r15 > ffffffff8a701772: 41 56 push %r14 > ffffffff8a701774: 41 55 push %r13 > ffffffff8a701776: 41 54 push %r12 > > There are 5 parameters in original function, but in the cloned function > only 4. > > I believe this patch also fixes this oops bug[1], which arises in the > same function __smc_diag_dump. But I couldn't verify it further. Can > someone please test this? > > [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 Unfortunately, I tested it myself and this bug is still triggering. Basically, this bug is not triggered in normal situations, but triggered when a race condition occurs, so I think the root cause is somewhere else. Regards, Jeongjun Park > --- > net/smc/smc_diag.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c > index 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 > --- a/net/smc/smc_diag.c > +++ b/net/smc/smc_diag.c > @@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb, > > static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, > struct netlink_callback *cb, > - const struct smc_diag_req *req, > - struct nlattr *bc) > + const struct smc_diag_req *req) > { > struct smc_sock *smc = smc_sk(sk); > struct smc_diag_fallback fallback; > @@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, > struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); > struct net *net = sock_net(skb->sk); > int snum = cb_ctx->pos[p_type]; > - struct nlattr *bc = NULL; > struct hlist_head *head; > int rc = 0, num = 0; > struct sock *sk; > @@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, > continue; > if (num < snum) > goto next; > - rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); > + rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); > if (rc < 0) > goto out; > next: > > --- > base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230 > change-id: 20241109-fix-oops-__smc_diag_dump-06ab3e9d39f4 > > Best regards, > -- > Manas <manas18244@iiitd.ac.in>
On 09.11.24 07:28, Manas via B4 Relay wrote: > From: Manas <manas18244@iiitd.ac.in> > > The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. > There is only one instance of this function being called and its passed > with a NULL value in place of bc. > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > --- > The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. > There is only one instance of this function being called and its passed > with a NULL value in place of bc. > > Though, the compiler (gcc) optimizes it. Looking at the object dump of > vmlinux (via `objdump -D vmlinux`), a new function clone > (__smc_diag_dump.constprop.0) is added which removes this parameter from > calling convention altogether. > > ffffffff8a701770 <__smc_diag_dump.constprop.0>: > ffffffff8a701770: 41 57 push %r15 > ffffffff8a701772: 41 56 push %r14 > ffffffff8a701774: 41 55 push %r13 > ffffffff8a701776: 41 54 push %r12 > > There are 5 parameters in original function, but in the cloned function > only 4. > > I believe this patch also fixes this oops bug[1], which arises in the > same function __smc_diag_dump. But I couldn't verify it further. Can > someone please test this? > > [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 > --- > net/smc/smc_diag.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c > index 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 > --- a/net/smc/smc_diag.c > +++ b/net/smc/smc_diag.c > @@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb, > > static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, > struct netlink_callback *cb, > - const struct smc_diag_req *req, > - struct nlattr *bc) > + const struct smc_diag_req *req) > { > struct smc_sock *smc = smc_sk(sk); > struct smc_diag_fallback fallback; > @@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, > struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); > struct net *net = sock_net(skb->sk); > int snum = cb_ctx->pos[p_type]; > - struct nlattr *bc = NULL; > struct hlist_head *head; > int rc = 0, num = 0; > struct sock *sk; > @@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, > continue; > if (num < snum) > goto next; > - rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); > + rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); > if (rc < 0) > goto out; > next: > > --- > base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230 > change-id: 20241109-fix-oops-__smc_diag_dump-06ab3e9d39f4 > > Best regards, That's true that the last parameter is not used. And the patch you suggested as a cleanup patch looks good to me. However, it should not fix the bug[1], because it does not match what the bug[1] described. Thank you, Jeongjun, for testing it! That verified that it indeed didn't fix the issue. I think the root cause is on handling idiag_sport. I'll look into it. [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 Thanks, Wenjia
On 11.11.2024 22:04, Jeongjun Park wrote: >Manas <devnull+manas18244.iiitd.ac.in@kernel.org> wrote: >> The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >> There is only one instance of this function being called and its passed >> with a NULL value in place of bc. >> >> Signed-off-by: Manas <manas18244@iiitd.ac.in> >> --- >> The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >> There is only one instance of this function being called and its passed >> with a NULL value in place of bc. >> >> Though, the compiler (gcc) optimizes it. Looking at the object dump of >> vmlinux (via `objdump -D vmlinux`), a new function clone >> (__smc_diag_dump.constprop.0) is added which removes this parameter from >> calling convention altogether. >> >> ffffffff8a701770 <__smc_diag_dump.constprop.0>: >> ffffffff8a701770: 41 57 push %r15 >> ffffffff8a701772: 41 56 push %r14 >> ffffffff8a701774: 41 55 push %r13 >> ffffffff8a701776: 41 54 push %r12 >> >> There are 5 parameters in original function, but in the cloned function >> only 4. >> >> I believe this patch also fixes this oops bug[1], which arises in the >> same function __smc_diag_dump. But I couldn't verify it further. Can >> someone please test this? >> >> [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 > >Unfortunately, I tested it myself and this bug is still triggering. Basically, >this bug is not triggered in normal situations, but triggered when a race >condition occurs, so I think the root cause is somewhere else. > Thank you Jeongjun for reviewing this. >> --- >> net/smc/smc_diag.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c >> index 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 >> --- a/net/smc/smc_diag.c >> +++ b/net/smc/smc_diag.c >> @@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb, >> >> static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, >> struct netlink_callback *cb, >> - const struct smc_diag_req *req, >> - struct nlattr *bc) >> + const struct smc_diag_req *req) >> { >> struct smc_sock *smc = smc_sk(sk); >> struct smc_diag_fallback fallback; >> @@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, >> struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); >> struct net *net = sock_net(skb->sk); >> int snum = cb_ctx->pos[p_type]; >> - struct nlattr *bc = NULL; >> struct hlist_head *head; >> int rc = 0, num = 0; >> struct sock *sk; >> @@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, >> continue; >> if (num < snum) >> goto next; >> - rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); >> + rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); >> if (rc < 0) >> goto out; >> next: >> >> --- >> base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230 >> change-id: 20241109-fix-oops-__smc_diag_dump-06ab3e9d39f4 >> >> Best regards, >> -- >> Manas <manas18244@iiitd.ac.in>
On 11.11.2024 15:11, Wenjia Zhang wrote: > > >On 09.11.24 07:28, Manas via B4 Relay wrote: >>From: Manas <manas18244@iiitd.ac.in> >> >>The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >>There is only one instance of this function being called and its passed >>with a NULL value in place of bc. >> >>Signed-off-by: Manas <manas18244@iiitd.ac.in> >>--- >>The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >>There is only one instance of this function being called and its passed >>with a NULL value in place of bc. >> >>Though, the compiler (gcc) optimizes it. Looking at the object dump of >>vmlinux (via `objdump -D vmlinux`), a new function clone >>(__smc_diag_dump.constprop.0) is added which removes this parameter from >>calling convention altogether. >> >>ffffffff8a701770 <__smc_diag_dump.constprop.0>: >>ffffffff8a701770: 41 57 push %r15 >>ffffffff8a701772: 41 56 push %r14 >>ffffffff8a701774: 41 55 push %r13 >>ffffffff8a701776: 41 54 push %r12 >> >>There are 5 parameters in original function, but in the cloned function >>only 4. >> >>I believe this patch also fixes this oops bug[1], which arises in the >>same function __smc_diag_dump. But I couldn't verify it further. Can >>someone please test this? >> >>[1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 >>--- >> net/smc/smc_diag.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >>diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c >>index 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 >>--- a/net/smc/smc_diag.c >>+++ b/net/smc/smc_diag.c >>@@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb, >> static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, >> struct netlink_callback *cb, >>- const struct smc_diag_req *req, >>- struct nlattr *bc) >>+ const struct smc_diag_req *req) >> { >> struct smc_sock *smc = smc_sk(sk); >> struct smc_diag_fallback fallback; >>@@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, >> struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); >> struct net *net = sock_net(skb->sk); >> int snum = cb_ctx->pos[p_type]; >>- struct nlattr *bc = NULL; >> struct hlist_head *head; >> int rc = 0, num = 0; >> struct sock *sk; >>@@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, >> continue; >> if (num < snum) >> goto next; >>- rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); >>+ rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); >> if (rc < 0) >> goto out; >> next: >> >>--- >>base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230 >>change-id: 20241109-fix-oops-__smc_diag_dump-06ab3e9d39f4 >> >>Best regards, > >That's true that the last parameter is not used. And the patch you >suggested as a cleanup patch looks good to me. However, it should not >fix the bug[1], because it does not match what the bug[1] described. >Thank you, Jeongjun, for testing it! That verified that it indeed >didn't fix the issue. I think the root cause is on handling >idiag_sport. I'll look into it. > >[1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 > >Thanks, >Wenjia Thank you Wenjia for reviewing this. Should I make any changes to the commit message if we are going forward with it being as a cleanup patch? The commit message itself (barring the cover letter) should be enough, I reckon.
On 11.11.24 16:10, Manas wrote: > On 11.11.2024 15:11, Wenjia Zhang wrote: >> >> >> On 09.11.24 07:28, Manas via B4 Relay wrote: >>> From: Manas <manas18244@iiitd.ac.in> >>> >>> The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >>> There is only one instance of this function being called and its passed >>> with a NULL value in place of bc. >>> >>> Signed-off-by: Manas <manas18244@iiitd.ac.in> >>> --- >>> The last parameter in __smc_diag_dump (struct nlattr *bc) is unused. >>> There is only one instance of this function being called and its passed >>> with a NULL value in place of bc. >>> >>> Though, the compiler (gcc) optimizes it. Looking at the object dump of >>> vmlinux (via `objdump -D vmlinux`), a new function clone >>> (__smc_diag_dump.constprop.0) is added which removes this parameter from >>> calling convention altogether. >>> >>> ffffffff8a701770 <__smc_diag_dump.constprop.0>: >>> ffffffff8a701770: 41 57 push %r15 >>> ffffffff8a701772: 41 56 push %r14 >>> ffffffff8a701774: 41 55 push %r13 >>> ffffffff8a701776: 41 54 push %r12 >>> >>> There are 5 parameters in original function, but in the cloned function >>> only 4. >>> >>> I believe this patch also fixes this oops bug[1], which arises in the >>> same function __smc_diag_dump. But I couldn't verify it further. Can >>> someone please test this? >>> >>> [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 >>> --- >>> net/smc/smc_diag.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c >>> index >>> 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 >>> --- a/net/smc/smc_diag.c >>> +++ b/net/smc/smc_diag.c >>> @@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, >>> struct sk_buff *skb, >>> static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, >>> struct netlink_callback *cb, >>> - const struct smc_diag_req *req, >>> - struct nlattr *bc) >>> + const struct smc_diag_req *req) >>> { >>> struct smc_sock *smc = smc_sk(sk); >>> struct smc_diag_fallback fallback; >>> @@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto >>> *prot, struct sk_buff *skb, >>> struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); >>> struct net *net = sock_net(skb->sk); >>> int snum = cb_ctx->pos[p_type]; >>> - struct nlattr *bc = NULL; >>> struct hlist_head *head; >>> int rc = 0, num = 0; >>> struct sock *sk; >>> @@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto >>> *prot, struct sk_buff *skb, >>> continue; >>> if (num < snum) >>> goto next; >>> - rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); >>> + rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); >>> if (rc < 0) >>> goto out; >>> next: >>> >>> --- >>> base-commit: 59b723cd2adbac2a34fc8e12c74ae26ae45bf230 >>> change-id: 20241109-fix-oops-__smc_diag_dump-06ab3e9d39f4 >>> >>> Best regards, >> >> That's true that the last parameter is not used. And the patch you >> suggested as a cleanup patch looks good to me. However, it should not >> fix the bug[1], because it does not match what the bug[1] described. >> Thank you, Jeongjun, for testing it! That verified that it indeed >> didn't fix the issue. I think the root cause is on handling >> idiag_sport. I'll look into it. >> >> [1] https://syzkaller.appspot.com/bug?extid=271fed3ed6f24600c364 >> >> Thanks, >> Wenjia > > Thank you Wenjia for reviewing this. > > Should I make any changes to the commit message if we are going forward > with it > being as a cleanup patch? The commit message itself (barring the cover > letter) > should be enough, I reckon. > I think it is ok as it is. Thanks, Wenjia
On Tue, Nov 12, 2024 at 08:36:13AM +0100, Wenjia Zhang wrote: > > > On 11.11.24 16:10, Manas wrote: > > On 11.11.2024 15:11, Wenjia Zhang wrote: ... > > Thank you Wenjia for reviewing this. > > > > Should I make any changes to the commit message if we are going forward > > with it > > being as a cleanup patch? The commit message itself (barring the cover > > letter) > > should be enough, I reckon. > > > I think it is ok as it is. Yes, agreed. The commit message should be truncated at first scissors ("---"). Which leaves us with a commit message only describing the removal of an unused function parameter. Which, given the discussion in this thread, is what we want. Reviewed-by: Simon Horman <horms@kernel.org> > > Thanks, > Wenjia >
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c index 6fdb2d96777ad704c394709ec845f9ddef5e599a..8f7bd40f475945171a0afa5a2cce12d9aa2b1eb4 100644 --- a/net/smc/smc_diag.c +++ b/net/smc/smc_diag.c @@ -71,8 +71,7 @@ static int smc_diag_msg_attrs_fill(struct sock *sk, struct sk_buff *skb, static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb, struct netlink_callback *cb, - const struct smc_diag_req *req, - struct nlattr *bc) + const struct smc_diag_req *req) { struct smc_sock *smc = smc_sk(sk); struct smc_diag_fallback fallback; @@ -199,7 +198,6 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, struct smc_diag_dump_ctx *cb_ctx = smc_dump_context(cb); struct net *net = sock_net(skb->sk); int snum = cb_ctx->pos[p_type]; - struct nlattr *bc = NULL; struct hlist_head *head; int rc = 0, num = 0; struct sock *sk; @@ -214,7 +212,7 @@ static int smc_diag_dump_proto(struct proto *prot, struct sk_buff *skb, continue; if (num < snum) goto next; - rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh), bc); + rc = __smc_diag_dump(sk, skb, cb, nlmsg_data(cb->nlh)); if (rc < 0) goto out; next: