[PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[Gagan Kumar]

Add neighbour source flag in mctp_neigh_remove(...) to allow removal of
only static neighbours.

Signed-off-by: Gagan Kumar <gagan1kumar.cs@gmail.com>
---
 net/mctp/neigh.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mctp/neigh.c b/net/mctp/neigh.c
index 5cc042121493..a90723ae66d7 100644
--- a/net/mctp/neigh.c
+++ b/net/mctp/neigh.c
@@ -85,8 +85,8 @@ void mctp_neigh_remove_dev(struct mctp_dev *mdev)
 	mutex_unlock(&net->mctp.neigh_lock);
 }
 
-// TODO: add a "source" flag so netlink can only delete static neighbours?
-static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
+static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid,
+			     enum mctp_neigh_source source)
 {
 	struct net *net = dev_net(mdev->dev);
 	struct mctp_neigh *neigh, *tmp;
@@ -94,7 +94,7 @@ static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
 
 	mutex_lock(&net->mctp.neigh_lock);
 	list_for_each_entry_safe(neigh, tmp, &net->mctp.neighbours, list) {
-		if (neigh->dev == mdev && neigh->eid == eid) {
+		if (neigh->dev == mdev && neigh->eid == eid && neigh->source == source) {
 			list_del_rcu(&neigh->list);
 			/* TODO: immediate RTM_DELNEIGH */
 			call_rcu(&neigh->rcu, __mctp_neigh_free);
@@ -202,7 +202,7 @@ static int mctp_rtm_delneigh(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!mdev)
 		return -ENODEV;
 
-	return mctp_neigh_remove(mdev, eid);
+	return mctp_neigh_remove(mdev, eid, MCTP_NEIGH_STATIC);
 }
 
 static int mctp_fill_neigh(struct sk_buff *skb, u32 portid, u32 seq, int event,
-- 
2.32.0

Re: [PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[Jakub Kicinski]

On Tue, 28 Dec 2021 18:39:56 +0530 Gagan Kumar wrote:
> Add neighbour source flag in mctp_neigh_remove(...) to allow removal of
> only static neighbours.

Which are the only ones that exist today right?

Can you clarify the motivation and practical impact of the change 
in the commit message to make it clear? AFAICT this is a no-op / prep
for some later changes, right Jeremy?

> diff --git a/net/mctp/neigh.c b/net/mctp/neigh.c
> index 5cc042121493..a90723ae66d7 100644
> --- a/net/mctp/neigh.c
> +++ b/net/mctp/neigh.c
> @@ -85,8 +85,8 @@ void mctp_neigh_remove_dev(struct mctp_dev *mdev)
>  	mutex_unlock(&net->mctp.neigh_lock);
>  }
>  
> -// TODO: add a "source" flag so netlink can only delete static neighbours?
> -static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
> +static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid,
> +			     enum mctp_neigh_source source)
>  {
>  	struct net *net = dev_net(mdev->dev);
>  	struct mctp_neigh *neigh, *tmp;
> @@ -94,7 +94,7 @@ static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
>  
>  	mutex_lock(&net->mctp.neigh_lock);
>  	list_for_each_entry_safe(neigh, tmp, &net->mctp.neighbours, list) {
> -		if (neigh->dev == mdev && neigh->eid == eid) {
> +		if (neigh->dev == mdev && neigh->eid == eid && neigh->source == source) {
>  			list_del_rcu(&neigh->list);
>  			/* TODO: immediate RTM_DELNEIGH */
>  			call_rcu(&neigh->rcu, __mctp_neigh_free);
> @@ -202,7 +202,7 @@ static int mctp_rtm_delneigh(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (!mdev)
>  		return -ENODEV;
>  
> -	return mctp_neigh_remove(mdev, eid);
> +	return mctp_neigh_remove(mdev, eid, MCTP_NEIGH_STATIC);
>  }
>  
>  static int mctp_fill_neigh(struct sk_buff *skb, u32 portid, u32 seq, int event,

Re: [PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[Jeremy Kerr]

Hi Jakub & Gagan,

> > Add neighbour source flag in mctp_neigh_remove(...) to allow
> > removal of only static neighbours.
> 
> Which are the only ones that exist today right?

That's correct. There may be a future facility for the kernel to perform
neighbour discovery itself (somewhat analogous to ARP), but only the
static entries are possible at the moment.

> Can you clarify the motivation and practical impact of the change 
> in the commit message to make it clear? AFAICT this is a no-op / prep
> for some later changes, right Jeremy?

Yes, it'll be a no-op now; I'm not aware of any changes coming that
require parameterisation of the neighbour type yet.

Gagan - can you provide any context on this change?

Cheers,


Jeremy

Re: [PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[Gagan Kumar]

Hi Jeremy and Jakub,

Thanks for the response.

On Fri, 2021-12-31 at 11:33 +0800, Jeremy Kerr wrote:

> Hi Jakub & Gagan,
> 
> > > Add neighbour source flag in mctp_neigh_remove(...) to allow
> > > removal of only static neighbours.
> > 
> > Which are the only ones that exist today right?
> 
> That's correct. There may be a future facility for the kernel to
> perform
> neighbour discovery itself (somewhat analogous to ARP), but only the
> static entries are possible at the moment.
> 
> > Can you clarify the motivation and practical impact of the change 
> > in the commit message to make it clear? AFAICT this is a no-op / prep
> > for some later changes, right Jeremy?
> 
> Yes, it'll be a no-op now; I'm not aware of any changes coming that
> require parameterisation of the neighbour type yet.
> 
> Gagan - can you provide any context on this change?

I was exploring the repository and wanted to get familiar with the
patching process. During that, I was looking for some TODOs in /net for
my first patch and came across mctp.

I thought `TODO: add a "source" flag so netlink can only delete static
neighbours?` might be of some use in the future. So, thought of sending
a patch for the same.

If I were to think like a critic, "You aren't gonna need it" principle
can be applied here.

If you think it's ok to proceed I can update the commit message to
include the motivation and impact as a no-op.

> 
> Cheers,
> 
> 
> Jeremy

Thanks,
Gagan

Re: [PATCH] mctp: Remove only static neighbour on RTM_DELNEIGH

[Jakub Kicinski]

On Fri, 31 Dec 2021 17:44:15 +0530 Gagan Kumar wrote:
> > > Can you clarify the motivation and practical impact of the change 
> > > in the commit message to make it clear? AFAICT this is a no-op / prep
> > > for some later changes, right Jeremy?  
> > 
> > Yes, it'll be a no-op now; I'm not aware of any changes coming that
> > require parameterisation of the neighbour type yet.
> > 
> > Gagan - can you provide any context on this change?  
> 
> I was exploring the repository and wanted to get familiar with the
> patching process. During that, I was looking for some TODOs in /net for
> my first patch and came across mctp.
> 
> I thought `TODO: add a "source" flag so netlink can only delete static
> neighbours?` might be of some use in the future. So, thought of sending
> a patch for the same.
> 
> If I were to think like a critic, "You aren't gonna need it" principle
> can be applied here.
> 
> If you think it's ok to proceed I can update the commit message to
> include the motivation and impact as a no-op.

SGTM

[PATCH v2] mctp: Remove only static neighbour on RTM_DELNEIGH

[Gagan Kumar]

Add neighbour source flag in mctp_neigh_remove(...) to allow removal of
only static neighbours.

This should be a no-op change and might be useful later when mctp can
have MCTP_NEIGH_DISCOVER neighbours.

Signed-off-by: Gagan Kumar <gagan1kumar.cs@gmail.com>
---
Changes in v2:
  - Add motivation and impact in the commit message.
  - Split long line > 80 chars into two.

 net/mctp/neigh.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/mctp/neigh.c b/net/mctp/neigh.c
index 5cc042121493..6ad3e33bd4d4 100644
--- a/net/mctp/neigh.c
+++ b/net/mctp/neigh.c
@@ -85,8 +85,8 @@ void mctp_neigh_remove_dev(struct mctp_dev *mdev)
 	mutex_unlock(&net->mctp.neigh_lock);
 }
 
-// TODO: add a "source" flag so netlink can only delete static neighbours?
-static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
+static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid,
+			     enum mctp_neigh_source source)
 {
 	struct net *net = dev_net(mdev->dev);
 	struct mctp_neigh *neigh, *tmp;
@@ -94,7 +94,8 @@ static int mctp_neigh_remove(struct mctp_dev *mdev, mctp_eid_t eid)
 
 	mutex_lock(&net->mctp.neigh_lock);
 	list_for_each_entry_safe(neigh, tmp, &net->mctp.neighbours, list) {
-		if (neigh->dev == mdev && neigh->eid == eid) {
+		if (neigh->dev == mdev && neigh->eid == eid &&
+		    neigh->source == source) {
 			list_del_rcu(&neigh->list);
 			/* TODO: immediate RTM_DELNEIGH */
 			call_rcu(&neigh->rcu, __mctp_neigh_free);
@@ -202,7 +203,7 @@ static int mctp_rtm_delneigh(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!mdev)
 		return -ENODEV;
 
-	return mctp_neigh_remove(mdev, eid);
+	return mctp_neigh_remove(mdev, eid, MCTP_NEIGH_STATIC);
 }
 
 static int mctp_fill_neigh(struct sk_buff *skb, u32 portid, u32 seq, int event,
-- 
2.32.0

Re: [PATCH v2] mctp: Remove only static neighbour on RTM_DELNEIGH

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sat,  1 Jan 2022 11:11:25 +0530 you wrote:
> Add neighbour source flag in mctp_neigh_remove(...) to allow removal of
> only static neighbours.
> 
> This should be a no-op change and might be useful later when mctp can
> have MCTP_NEIGH_DISCOVER neighbours.
> 
> Signed-off-by: Gagan Kumar <gagan1kumar.cs@gmail.com>
> 
> [...]

Here is the summary with links:
  - [v2] mctp: Remove only static neighbour on RTM_DELNEIGH
    https://git.kernel.org/netdev/net/c/ae81de737885

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html