[PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[Alan Robertson]

ndo_dflt_fdb_del is checking for a condition which is opposite that
which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that is, if the
entry is static.  This is consistent with the failure error message.

On the other hand, ndo_dflt_del() declares an error
if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
precondition, and inconsistent with its failure message text.
As it is now, you can't delete any entry which add allows to be added -
so entry deletion always fails.

Signed-off-by: Alan Robertson <alanr@unix.sh>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index a08bd2b..373a8e7 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2142,7 +2142,7 @@ int ndo_dflt_fdb_del(struct ndmsg *ndm,
 	/* If aging addresses are supported device will need to
 	 * implement its own handler for this.
 	 */
-	if (ndm->ndm_state & NUD_PERMANENT) {
+	if (!(ndm->ndm_state & NUD_PERMANENT)) {
 		pr_info("%s: FDB only supports static addresses\n", dev->name);
 		return -EINVAL;
 	}
-- 
1.8.1.4

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[David Miller]

From: Alan Robertson <alanr@unix.sh>
Date: Thu, 30 May 2013 16:01:55 -0600

> ndo_dflt_fdb_del is checking for a condition which is opposite that
> which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
> if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that is, if the
> entry is static.  This is consistent with the failure error message.
> 
> On the other hand, ndo_dflt_del() declares an error
> if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
> precondition, and inconsistent with its failure message text.
> As it is now, you can't delete any entry which add allows to be added -
> so entry deletion always fails.
> 
> Signed-off-by: Alan Robertson <alanr@unix.sh>

What about the ->ndm_state part of the add() test?  Why not include
that in the del() check?

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[Alan Robertson]

On 05/31/2013 02:46 AM, David Miller wrote:
> From: Alan Robertson <alanr@unix.sh>
> Date: Thu, 30 May 2013 16:01:55 -0600
>
>> ndo_dflt_fdb_del is checking for a condition which is opposite that
>> which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
>> if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that is, if the
>> entry is static.  This is consistent with the failure error message.
>>
>> On the other hand, ndo_dflt_del() declares an error
>> if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
>> precondition, and inconsistent with its failure message text.
>> As it is now, you can't delete any entry which add allows to be added -
>> so entry deletion always fails.
>>
>> Signed-off-by: Alan Robertson <alanr@unix.sh>
> What about the ->ndm_state part of the add() test?  Why not include
> that in the del() check?
I had three different thoughts about this:
  1) Replicated the add check in the delete
  2) Do what I did - make it where you can only delete those really
marked as static
  3) Eliminate all delete checks

The problem is -- I'm not sure why the ndm->ndm_state check is in there
-- I don't know how to make that condition occur.  It has the feel of a
check to be made before things are fully initialized - or maybe even
just leftover cruft.

You could make the argument that option (3) is the best -- if it's been
added successfully, you ought to be able to delete it.

If someone with more knowledge would comment that would be much appreciated!

I'm happy to submit any of these three versions of the patch.  Let me
know what's wanted.


-- 
    Alan Robertson <alanr@unix.sh> - @OSSAlanR

"Openness is the foundation and preservative of friendship...  Let me claim from you at all times your undisguised opinions." - William Wilberforce

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[Vlad Yasevich]

On 05/31/2013 12:43 PM, Alan Robertson wrote:
> On 05/31/2013 02:46 AM, David Miller wrote:
>> From: Alan Robertson <alanr@unix.sh>
>> Date: Thu, 30 May 2013 16:01:55 -0600
>>
>>> ndo_dflt_fdb_del is checking for a condition which is opposite that
>>> which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
>>> if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that is, if the
>>> entry is static.  This is consistent with the failure error message.
>>>
>>> On the other hand, ndo_dflt_del() declares an error
>>> if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
>>> precondition, and inconsistent with its failure message text.
>>> As it is now, you can't delete any entry which add allows to be added -
>>> so entry deletion always fails.
>>>
>>> Signed-off-by: Alan Robertson <alanr@unix.sh>
>> What about the ->ndm_state part of the add() test?  Why not include
>> that in the del() check?
> I had three different thoughts about this:
>    1) Replicated the add check in the delete
>    2) Do what I did - make it where you can only delete those really
> marked as static
>    3) Eliminate all delete checks
>
> The problem is -- I'm not sure why the ndm->ndm_state check is in there
> -- I don't know how to make that condition occur.  It has the feel of a
> check to be made before things are fully initialized - or maybe even
> just leftover cruft.
>

The test  is there to support simultaneous master and self operations. 
The operation on a master may not always require a NUD_PERMANENT state 
(ex: bridge) and we don't want to perform self operations in that instance.

> You could make the argument that option (3) is the best -- if it's been
> added successfully, you ought to be able to delete it.

Hmm..  _del() always deletes and flags/states don't matter much 
generally.  I agree that the check isn't really needed as it isn't 
really protecting anything.

-vlad

>
> If someone with more knowledge would comment that would be much appreciated!
>
> I'm happy to submit any of these three versions of the patch.  Let me
> know what's wanted.
>
>

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[Alan Robertson]

On 05/31/2013 11:11 AM, Vlad Yasevich wrote:
> On 05/31/2013 12:43 PM, Alan Robertson wrote:
>> On 05/31/2013 02:46 AM, David Miller wrote:
>>> From: Alan Robertson <alanr@unix.sh>
>>> Date: Thu, 30 May 2013 16:01:55 -0600
>>>
>>>> ndo_dflt_fdb_del is checking for a condition which is opposite that
>>>> which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
>>>> if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that
>>>> is, if the
>>>> entry is static.  This is consistent with the failure error message.
>>>>
>>>> On the other hand, ndo_dflt_del() declares an error
>>>> if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
>>>> precondition, and inconsistent with its failure message text.
>>>> As it is now, you can't delete any entry which add allows to be
>>>> added -
>>>> so entry deletion always fails.
>>>>
>>>> Signed-off-by: Alan Robertson <alanr@unix.sh>
>>> What about the ->ndm_state part of the add() test?  Why not include
>>> that in the del() check?
>> I had three different thoughts about this:
>>    1) Replicated the add check in the delete
>>    2) Do what I did - make it where you can only delete those really
>> marked as static
>>    3) Eliminate all delete checks
>>
>> The problem is -- I'm not sure why the ndm->ndm_state check is in there
>> -- I don't know how to make that condition occur.  It has the feel of a
>> check to be made before things are fully initialized - or maybe even
>> just leftover cruft.
>>
>
> The test  is there to support simultaneous master and self operations.
> The operation on a master may not always require a NUD_PERMANENT state
> (ex: bridge) and we don't want to perform self operations in that
> instance.
>
>> You could make the argument that option (3) is the best -- if it's been
>> added successfully, you ought to be able to delete it.
>
> Hmm..  _del() always deletes and flags/states don't matter much
> generally.  I agree that the check isn't really needed as it isn't
> really protecting anything.

So is that what folks want me to do?

-- 
    Alan Robertson <alanr@unix.sh> - @OSSAlanR

"Openness is the foundation and preservative of friendship...  Let me claim from you at all times your undisguised opinions." - William Wilberforce

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[David Miller]

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Fri, 31 May 2013 13:11:47 -0400

> On 05/31/2013 12:43 PM, Alan Robertson wrote:
>> On 05/31/2013 02:46 AM, David Miller wrote:
>>> From: Alan Robertson <alanr@unix.sh>
>>> Date: Thu, 30 May 2013 16:01:55 -0600
>>>
>>>> ndo_dflt_fdb_del is checking for a condition which is opposite that
>>>> which ndo_dflt_fdb_add enforces.  ndo_dflt_fdb_add declares an error
>>>> if (ndm->ndm_state && !(ndm->ndm_state) & NUD_PERMANENT)) - that is,
>>>> if the
>>>> entry is static.  This is consistent with the failure error message.
>>>>
>>>> On the other hand, ndo_dflt_del() declares an error
>>>> if (ndm_state & NUD_PERMANENT) - which is inconsistent with the add
>>>> precondition, and inconsistent with its failure message text.
>>>> As it is now, you can't delete any entry which add allows to be added
>>>> -
>>>> so entry deletion always fails.
>>>>
>>>> Signed-off-by: Alan Robertson <alanr@unix.sh>
>>> What about the ->ndm_state part of the add() test?  Why not include
>>> that in the del() check?
>> I had three different thoughts about this:
>>    1) Replicated the add check in the delete
>>    2) Do what I did - make it where you can only delete those really
>> marked as static
>>    3) Eliminate all delete checks
>>
>> The problem is -- I'm not sure why the ndm->ndm_state check is in
>> there
>> -- I don't know how to make that condition occur.  It has the feel of a
>> check to be made before things are fully initialized - or maybe even
>> just leftover cruft.
>>
> 
> The test is there to support simultaneous master and self
> operations. The operation on a master may not always require a
> NUD_PERMANENT state (ex: bridge) and we don't want to perform self
> operations in that instance.

I still don't understand the ndm_state check.  Please use different
words to explain it so that even an idiot like me can understand.

Once we define what the check should exactly be I propose:

1) Keeping the check only in add()

2) Removing the state checks completely in del()

3) Validating at netdevice registry time or elsewhere that these
   default fdb ops are always used together.  That wraps up everything
   to ensure that only doing the check in add() is provably correct.

Re: [PATCH net] rtnetlink: ndo_dflt_fdb_del() never works

[Alan Robertson]

I'd like to fix this problem - but it seems good to answer David
Miller's question before we decide which way to go.  I certainly don't
know the answer...

I can do (1) and (2), but I'm not sure how to do (3) properly.

Vlad:  Can you help out here?


On 05/31/2013 05:42 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Fri, 31 May 2013 13:11:47 -0400
>
<snip>
>> The test is there to support simultaneous master and self
>> operations. The operation on a master may not always require a
>> NUD_PERMANENT state (ex: bridge) and we don't want to perform self
>> operations in that instance.
> I still don't understand the ndm_state check.  Please use different
> words to explain it so that even an idiot like me can understand.
>
> Once we define what the check should exactly be I propose:
>
> 1) Keeping the check only in add()
>
> 2) Removing the state checks completely in del()
>
> 3) Validating at netdevice registry time or elsewhere that these
>    default fdb ops are always used together.  That wraps up everything
>    to ensure that only doing the check in add() is provably correct.
> --
> 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


-- 
    Alan Robertson <alanr@unix.sh> - @OSSAlanR

"Openness is the foundation and preservative of friendship...  Let me claim from you at all times your undisguised opinions." - William Wilberforce