Re: [PATCH] arp: Remove the arp_hh_ops structure
Thank you for the clarification. On Tue, Feb 23, 2021 at 12:07 AM David Ahern wrote: > > On 2/22/21 1:37 AM, Eric Dumazet wrote: > > > > > > On 2/22/21 4:15 AM, Yejune Deng wrote: > >> The arp_hh_ops structure is similar to the arp_generic_ops structure. > >> but the latter is more general,so remove the arp_hh_ops structure. > >> > >> Fix when took out the neigh->ops assignment: > >> 8.973653] #PF: supervisor read access in kernel mode > >> [8.975027] #PF: error_code(0x) - not-present page > >> [8.976310] PGD 0 P4D 0 > >> [8.977036] Oops: [#1] SMP PTI > >> [8.977973] CPU: 1 PID: 210 Comm: sd-resolve Not tainted > >> 5.11.0-rc7-02046-g4591591ab715 #1 > >> [8.979998] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > >> 1.12.0-1 04/01/2014 > >> [8.981996] RIP: 0010:neigh_probe > >> (kbuild/src/consumer/net/core/neighbour.c:1009) > >> > > > > I have a hard time understanding this patch submission. > > > > This seems a mix of a net-next and net material ? > > It is net-next at best. > > > > > > > > >> Reported-by: kernel test robot > > > > If this is a bug fix, we want a Fixes: tag > > > > This will really help us. Please don't let us guess what is going on. > > > > This patch is a v2. v1 was clearly wrong and not tested; I responded as > such 12 hours before the robot test.
Re: [PATCH] arp: Remove the arp_hh_ops structure
On 2/22/21 1:37 AM, Eric Dumazet wrote: > > > On 2/22/21 4:15 AM, Yejune Deng wrote: >> The arp_hh_ops structure is similar to the arp_generic_ops structure. >> but the latter is more general,so remove the arp_hh_ops structure. >> >> Fix when took out the neigh->ops assignment: >> 8.973653] #PF: supervisor read access in kernel mode >> [8.975027] #PF: error_code(0x) - not-present page >> [8.976310] PGD 0 P4D 0 >> [8.977036] Oops: [#1] SMP PTI >> [8.977973] CPU: 1 PID: 210 Comm: sd-resolve Not tainted >> 5.11.0-rc7-02046-g4591591ab715 #1 >> [8.979998] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> 1.12.0-1 04/01/2014 >> [8.981996] RIP: 0010:neigh_probe >> (kbuild/src/consumer/net/core/neighbour.c:1009) >> > > I have a hard time understanding this patch submission. > > This seems a mix of a net-next and net material ? It is net-next at best. > > > >> Reported-by: kernel test robot > > If this is a bug fix, we want a Fixes: tag > > This will really help us. Please don't let us guess what is going on. > This patch is a v2. v1 was clearly wrong and not tested; I responded as such 12 hours before the robot test.
Re: [PATCH] arp: Remove the arp_hh_ops structure
On 2/22/21 4:15 AM, Yejune Deng wrote: > The arp_hh_ops structure is similar to the arp_generic_ops structure. > but the latter is more general,so remove the arp_hh_ops structure. > > Fix when took out the neigh->ops assignment: > 8.973653] #PF: supervisor read access in kernel mode > [8.975027] #PF: error_code(0x) - not-present page > [8.976310] PGD 0 P4D 0 > [8.977036] Oops: [#1] SMP PTI > [8.977973] CPU: 1 PID: 210 Comm: sd-resolve Not tainted > 5.11.0-rc7-02046-g4591591ab715 #1 > [8.979998] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > 1.12.0-1 04/01/2014 > [8.981996] RIP: 0010:neigh_probe > (kbuild/src/consumer/net/core/neighbour.c:1009) > I have a hard time understanding this patch submission. This seems a mix of a net-next and net material ? > Reported-by: kernel test robot If this is a bug fix, we want a Fixes: tag This will really help us. Please don't let us guess what is going on. Also, if this is not a bug fix, this should target net-next tree, please take a look at Documentation/networking/netdev-FAQ.rst In short, the stack trace in this changelog seems to be not related to this patch, maybe a prior version ? We do not want to keep artifacts of some buggy version that was never merged into official tree. Thanks. > Signed-off-by: Yejune Deng > --- > net/ipv4/arp.c | 15 ++- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c > index 922dd73e5740..9ee59c2e419a 100644 > --- a/net/ipv4/arp.c > +++ b/net/ipv4/arp.c > @@ -135,14 +135,6 @@ static const struct neigh_ops arp_generic_ops = { > .connected_output = neigh_connected_output, > }; > > -static const struct neigh_ops arp_hh_ops = { > - .family = AF_INET, > - .solicit = arp_solicit, > - .error_report = arp_error_report, > - .output = neigh_resolve_output, > - .connected_output = neigh_resolve_output, > -}; > - > static const struct neigh_ops arp_direct_ops = { > .family = AF_INET, > .output = neigh_direct_output, > @@ -277,12 +269,9 @@ static int arp_constructor(struct neighbour *neigh) > memcpy(neigh->ha, dev->broadcast, dev->addr_len); > } > > - if (dev->header_ops->cache) > - neigh->ops = _hh_ops; > - else > - neigh->ops = _generic_ops; > + neigh->ops = _generic_ops; > > - if (neigh->nud_state & NUD_VALID) > + if (!dev->header_ops->cache && (neigh->nud_state & NUD_VALID)) > neigh->output = neigh->ops->connected_output; > else > neigh->output = neigh->ops->output; >
[PATCH] arp: Remove the arp_hh_ops structure
The arp_hh_ops structure is similar to the arp_generic_ops structure. but the latter is more general,so remove the arp_hh_ops structure. Fix when took out the neigh->ops assignment: 8.973653] #PF: supervisor read access in kernel mode [8.975027] #PF: error_code(0x) - not-present page [8.976310] PGD 0 P4D 0 [8.977036] Oops: [#1] SMP PTI [8.977973] CPU: 1 PID: 210 Comm: sd-resolve Not tainted 5.11.0-rc7-02046-g4591591ab715 #1 [8.979998] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 [8.981996] RIP: 0010:neigh_probe (kbuild/src/consumer/net/core/neighbour.c:1009) Reported-by: kernel test robot Signed-off-by: Yejune Deng --- net/ipv4/arp.c | 15 ++- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index 922dd73e5740..9ee59c2e419a 100644 --- a/net/ipv4/arp.c +++ b/net/ipv4/arp.c @@ -135,14 +135,6 @@ static const struct neigh_ops arp_generic_ops = { .connected_output = neigh_connected_output, }; -static const struct neigh_ops arp_hh_ops = { - .family = AF_INET, - .solicit = arp_solicit, - .error_report = arp_error_report, - .output = neigh_resolve_output, - .connected_output = neigh_resolve_output, -}; - static const struct neigh_ops arp_direct_ops = { .family = AF_INET, .output = neigh_direct_output, @@ -277,12 +269,9 @@ static int arp_constructor(struct neighbour *neigh) memcpy(neigh->ha, dev->broadcast, dev->addr_len); } - if (dev->header_ops->cache) - neigh->ops = _hh_ops; - else - neigh->ops = _generic_ops; + neigh->ops = _generic_ops; - if (neigh->nud_state & NUD_VALID) + if (!dev->header_ops->cache && (neigh->nud_state & NUD_VALID)) neigh->output = neigh->ops->connected_output; else neigh->output = neigh->ops->output; -- 2.29.0
Re: [PATCH] arp: Remove the arp_hh_ops structure
Sorry,it was my fault, I will resubmit. On Sun, Feb 21, 2021 at 9:54 AM David Ahern wrote: > > On 2/19/21 9:32 PM, Yejune Deng wrote: > > static const struct neigh_ops arp_direct_ops = { > > .family = AF_INET, > > .output = neigh_direct_output, > > @@ -277,15 +269,10 @@ static int arp_constructor(struct neighbour *neigh) > > memcpy(neigh->ha, dev->broadcast, dev->addr_len); > > } > > > > - if (dev->header_ops->cache) > > - neigh->ops = _hh_ops; > > - else > > - neigh->ops = _generic_ops; > > How did you test this? > > you took out the neigh->ops assignment, so all of the neigh->ops in > net/core/neighbour.c are going to cause a NULL dereference. > > > > - > > - if (neigh->nud_state & NUD_VALID) > > - neigh->output = neigh->ops->connected_output; > > + if (!dev->header_ops->cache && (neigh->nud_state & NUD_VALID)) > > + neigh->output = arp_generic_ops.connected_output; > > else > > - neigh->output = neigh->ops->output; > > + neigh->output = arp_generic_ops.output; > > } > > return 0; > > } > > >
Re: [PATCH] arp: Remove the arp_hh_ops structure
On 2/19/21 9:32 PM, Yejune Deng wrote: > static const struct neigh_ops arp_direct_ops = { > .family = AF_INET, > .output = neigh_direct_output, > @@ -277,15 +269,10 @@ static int arp_constructor(struct neighbour *neigh) > memcpy(neigh->ha, dev->broadcast, dev->addr_len); > } > > - if (dev->header_ops->cache) > - neigh->ops = _hh_ops; > - else > - neigh->ops = _generic_ops; How did you test this? you took out the neigh->ops assignment, so all of the neigh->ops in net/core/neighbour.c are going to cause a NULL dereference. > - > - if (neigh->nud_state & NUD_VALID) > - neigh->output = neigh->ops->connected_output; > + if (!dev->header_ops->cache && (neigh->nud_state & NUD_VALID)) > + neigh->output = arp_generic_ops.connected_output; > else > - neigh->output = neigh->ops->output; > + neigh->output = arp_generic_ops.output; > } > return 0; > } >
[PATCH] arp: Remove the arp_hh_ops structure
The 'arp_hh_ops' structure is similar to the 'arp_generic_ops' structure. So remove the 'arp_hh_ops' structure. Signed-off-by: Yejune Deng --- net/ipv4/arp.c | 19 +++ 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index 922dd73e5740..6d60d9b89286 100644 --- a/net/ipv4/arp.c +++ b/net/ipv4/arp.c @@ -135,14 +135,6 @@ static const struct neigh_ops arp_generic_ops = { .connected_output = neigh_connected_output, }; -static const struct neigh_ops arp_hh_ops = { - .family = AF_INET, - .solicit = arp_solicit, - .error_report = arp_error_report, - .output = neigh_resolve_output, - .connected_output = neigh_resolve_output, -}; - static const struct neigh_ops arp_direct_ops = { .family = AF_INET, .output = neigh_direct_output, @@ -277,15 +269,10 @@ static int arp_constructor(struct neighbour *neigh) memcpy(neigh->ha, dev->broadcast, dev->addr_len); } - if (dev->header_ops->cache) - neigh->ops = _hh_ops; - else - neigh->ops = _generic_ops; - - if (neigh->nud_state & NUD_VALID) - neigh->output = neigh->ops->connected_output; + if (!dev->header_ops->cache && (neigh->nud_state & NUD_VALID)) + neigh->output = arp_generic_ops.connected_output; else - neigh->output = neigh->ops->output; + neigh->output = arp_generic_ops.output; } return 0; } -- 2.29.0