Bug 10166 - HTB classes cannot be removed once a u32 filter has attached to the class, (even if the filter is first deleted)
Summary: HTB classes cannot be removed once a u32 filter has attached to the class, (e...
Status: CLOSED WILL_FIX_LATER
Alias: None
Product: Networking
Classification: Unclassified
Component: IPV4 (show other bugs)
Hardware: All Linux
: P1 low
Assignee: Stephen Hemminger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-03-04 08:34 UTC by Russell Davies
Modified: 2008-11-02 04:27 UTC (History)
0 users

See Also:
Kernel Version: 2.6.23.12
Subsystem:
Regression: ---
Bisected commit-id:


Attachments

Description Russell Davies 2008-03-04 08:34:17 UTC
Latest working kernel version: N/A
Earliest failing kernel version: 2.6.23.12 is where I first noticed the issue
Distribution: Fedora 7
Hardware Environment: Dual Core Pentium D
Software Environment: 2.6.23.15-80.fc7, tc

Problem Description:
The problem relates to manipulating HTB qdiscs and clases, and u32 filters using the "tc" tool.  I've created the following trivial qdisc/class structure:

       htb root qdisc (1:0) 
              |
       htb root class (1:1) 
              |
       htb child class (1:2)

I attach a u32 filter with one rule to the root qdisc (1:0).  I then add another u32 filter with one rule to the htb root class (1:1).  Two hashtable are created with automatically assigned handles, as expected. 

If I then try to delete the root class (1:1), I receive an error "RTNETLINK answers: Device or resource busy".  I receive the same error even If I delete the filter on the class (1:1) first.  

In order to delete the HTB class (1:1), after a u32 filter has been associated with it, it seems that I first remove the u32 filter attached to the root qdisc (1:0). 

Steps to reproduce:
Run the following bash script, observe the unexpected RTNetlink errors.

>>>>>>>>>>>>>>>>>>>
<script>
#!/bin/bash

# This script created the following tc structure, and attaches filters
# where indicated.  The script then makes various attempt to delete the
# htb root class (1:1), demonstrating that RTNetlink errors are thrown
# unless the u32 attached to 1:0 is deleted first.
#
# htb root qdisc (1:0) ... attach a filter here
#        |
# htb root class (1:1) ... attach a filter here
#        |
# htb child class (1:2)
#

##############################
# Create the qdisc and class tree
tc qdisc del dev eth1 root > /dev/null
tc qdisc add dev eth1 root handle 1: htb
tc class add dev eth1 parent 1: classid 1:1 htb rate 1000000
tc class add dev eth1 parent 1:1 classid 1:2 htb rate 1000000

##############################
# Add a filter to the root htb qdisc. The hashtable with the handle
# 800: is automatically created
tc filter add dev eth1 parent 1: protocol ip prio 1 handle ::1 \
       u32 classid 1:1 match ip tos 0x19 1e


##############################
# Add a filter to the root htb class. The hashtable with the handle
# 801: is automatically created
tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle ::1 \
       u32 classid 1:2 match ip tos 0x19 1e


#####################################
# Try to remove the root htb class
tc class del dev eth1 classid 1:1

# Expected result is that the htb root class and htb child are both removed
# Actual result is: RTNETLINK answers: Device or resource busy


#####################################
# Try to remove the hashtable that is associated with the root htb class
tc filter del dev eth1 parent 1:1 protocol ip prio 1 u32

# Expected result: filter 801:: is removed
# Actual result: filter 801:: is removed


#####################################
# Try again to remove the root htb class
tc class del dev eth1 classid 1:1

# Expected result is that the root htb class and htb child are removed
# Actual result is: RTNETLINK answers: Device or resource busy

#####################################
# Remove the child class first, to demonstrate that this is the sticking point
tc class del dev eth1 classid 1:2
tc class del dev eth1 classid 1:1

# Expected result is that the root htb class and htb child are removed
# Actual result is: 1) The htb child is removed
#                   2) RTNETLINK answers: Device or resource busy

#####################################
# Remove the entire u32 filter from the htb root qdisc, and show that
# the delete then works
tc filter del dev eth1 parent 1:0 protocol ip prio 1 u32
tc class del dev eth1 classid 1:1

# Expected result is that filter 800:: is removed and the htb root class
# are both removed.
# Actual result is: 1) filter 800:: is removed
#                   2) the htb root class is removed


<\script>
Comment 1 Stephen Hemminger 2008-03-04 08:51:30 UTC
That is the way HTB works because it does not want to get into dealing with
nested filters and deletion, if you want to enhance it to do proper reference
management please submit a patch.

Relevant code:

static int htb_delete(struct Qdisc *sch, unsigned long arg)
{
,,,
	// TODO: why don't allow to delete subtree ? references ? does
	// tc subsys quarantee us that in htb_destroy it holds no class
	// refs so that we can remove children safely there ?
	if (!list_empty(&cl->children) || cl->filter_cnt)
		return -EBUSY;
Comment 2 Russell Davies 2008-03-04 11:34:12 UTC
I want to make it clear to any future readers that, although it might be possible fix the perceived bug by modifying htb_delete( ... ) as suggested, I'm not 100% convinced that the root cause is actually in the referenced code.  

To my casual inspection, the referenced code appears to be the only place in the execution path of deleting an HTB class that can return -EBUSY.  Therefore, I'll run with the idea that this is where the error is being raised. 

Based on the referenced code, my now revised expectation is that an HTB class cannot be deleted if it either has children or is associated with any filters.  Conversely, my expectation is that the deletion of an HTB class that has no children and no associated filters should not return with an -EBUSY error.  

With the referenced code in mind, I've modified my test script to isolate the cases of having children and having filters, (see below for new script).

What I find is that, as expected, I cannot delete an HTB class that has children. The HTB class can be deleted if its children are deleted first. 

Likewise, as expected, an HTB class cannot be deleted if it is associated with a filter.  However, the class cannot be deleted even if the associated filter is deleted.  Therefore, the delete operation appears dependant upon the history of the HTB class, rather than what should be its current state.  

The conclusion that I infer from my results is that cl->filter_cnt must be incremented when a filter is added to the class; but it is not decremented when that filter is removed.  

So, why is there an apparent asymmetry in the behavior?  Is this still as designed, or an unintentional behavior? Am I right to expect that cl->filter_cnt should be decremented when the filter is removed?  If not, then why does it appear to increment when a filter is attached? What's the relationship?  


>>>>>>>>>>>>>>>>>>>>>>>>>>
<script>
#!/bin/bash

# This script intends to show that the deletion of htb classes that have been associated with
# a u32 filter does not occur as expected. 
#
#
# The following snippet of code is in the execution path when deleting htb classes.  The code appears 
# the be only place in the excecution path of deleting a htb class that can return -EBUSY. 
# 
#        if (!list_empty(&cl->children) || cl->filter_cnt)
#                return -EBUSY;
#
# Based on this code, the expectation is that a htb class cannot be deleted if it has children
# or if it is associated with filters.  Conversely, if the class has no children and no associated
# filters, then an attempt to delete an htb class should not return as -EBUSY. 
#
# Two tests are provided to show isolate the cases for a htb class with chidren and with filters. 



#################################
##################################
# TEST 1: show that htb class with no filters bu with children cannot be deleted unless the children are deleted first
#
# This test creates the following tc structure. No filters are attached. 
# The script then makes various attempt to delete the htb root class (1:1), 
# demonstrating that a htb cannot be deleted if it has children. 
#
# htb root qdisc (1:0) 
#        |
# htb root class (1:1) ... attach a filter here
#        |
# htb child class (1:2)
#

#---------------------------
# Create the qdisc and class tree
tc qdisc del dev eth1 root > /dev/null
tc qdisc add dev eth1 root handle 1: htb
tc class add dev eth1 parent 1: classid 1:1 htb rate 1000000
tc class add dev eth1 parent 1:1 classid 1:2 htb rate 1000000

#---------------------------
# Try to remove the root htb class
tc class del dev eth1 classid 1:1

# Expected condition: (!list_empty(&cl->children) || cl->filter_cnt) == true
# Expected result: RTNETLINK answers: Device or resource busy  
# Actual result is: RTNETLINK answers: Device or resource busy


#---------------------------
# Remove the child class first
tc class del dev eth1 classid 1:2
tc class del dev eth1 classid 1:1

# Expected condition: (!list_empty(&cl->children) || cl->filter_cnt) == false
# Expected result is that the root htb class and htb child are removed 
# Actual result is: 1) The htb child (1:2) is removed
#                   2) The htb root class (1:1) is removed 

#---------------------------
# Conlusions: QED for the case of deleting HTB classes that have children. 







#################################
##################################
# TEST 2: show that htb class with no children but with a filter cannot be deleted at all
#
# This test creates the following tc structure. The htb root class has no children. 
# filters are attached at the locations shown. The script makes various attempt to 
# delete the htb root class (1:1), demonstrating that a htb cannot be deleted even if 
# its associated filter is removed first. 
#
# htb root qdisc (1:0) ... attach a filter here
#        |
# htb root class (1:1) ... attach a filter here
#

#---------------------------
# Create the qdisc and class tree WITHOUT the child htb class (1:2).  In this 
# way class 1:1 never has children and thus cl->children is always empty.
tc qdisc del dev eth1 root > /dev/null
tc qdisc add dev eth1 root handle 1: htb
tc class add dev eth1 parent 1: classid 1:1 htb rate 1000000


#---------------------------
# Add a filter to the root htb qdisc. The hashtable with the handle
# 800: is automatically created
tc filter add dev eth1 parent 1: protocol ip prio 1 handle ::1 \
       u32 classid 1:1 match ip tos 0x19 1e


#---------------------------
# Add a filter to the root htb class. The hashtable with the handle
# 801: is automatically created
tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle ::1 \
       u32 classid 1:2 match ip tos 0x19 1e


#---------------------------
# Try to remove the root htb class
tc class del dev eth1 classid 1:1

# Expected condition: (!list_empty(&cl->children) || cl->filter_cnt) == true
# Expected result: RTNETLINK answers: Device or resource busy  
# Actual result is: RTNETLINK answers: Device or resource busy


#---------------------------
# Try to remove the hashtable that is associated with the root htb class
tc filter del dev eth1 parent 1:1 protocol ip prio 1 u32

# Expected result: filter 801:: is removed
# Actual result: filter 801:: is removed


#---------------------------
# Try again to remove the root htb class
# the class should no longer have filters 
tc class del dev eth1 classid 1:1

# Expected condition: (!list_empty(&cl->children) || cl->filter_cnt) == false
# Expected result: The root htb class should be deleted  
# Actual result is: RTNETLINK answers: Device or resource busy

#---------------------------
# Conclusion: cl->filter_cnt becomes non-zero once a filter as added to 
# an htb class, but does not return to zero when filters are removed.

<\script>
Comment 3 Russell Davies 2008-03-04 13:29:43 UTC
Apologies... no intention to spam on a low priority issue.  One last comment, and I'll drop the subject for now (or cure it ;o) ). 

cl->filter gets modified in two places in sch_htb.c, these being: 

01476   static unsigned long htb_bind_filter ( ... )
...
01490         if (cl)
01491                 cl->filter_cnt++;
01492         else
01493                 q->filter_cnt++;
01494         return (unsigned long)cl;

and (symmetrically):

01497   static void htb_unbind_filter  	( ... )
...
01502         if (cl)
01503                 cl->filter_cnt--;
01504         else
01505                 q->filter_cnt--;

This being so, then I have to assume that htb_unbind_filter is not being called by my attempts to remove the filter. So, if line (1) below causes htb_bind_filter(...) to be called against the class, then should line (2) cause htb_unbind_filter(...) to be called? If it should then that's the bug.  If not, then I guess I'm just winging about a missing feature. 

1) tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle ::1 \
       u32 classid 1:2 match ip tos 0x19 1e 
2) tc filter del dev eth1 parent 1:1 protocol ip prio 1 u32



I also notice the following comments in htb_bind_filter( ..) :

01481         /*if (cl && !cl->level) return 0;
01482            The line above used to be there to prevent attaching filters to
01483            leaves. But at least tc_index filter uses this just to get class
01484            for other reasons so that we have to allow for it.
01485            ----
01486            19.6.2002 As Werner explained it is ok - bind filter is just
01487            another way to "lock" the class - unlike "get" this lock can
01488            be broken by class during destroy IIUC.
01489          */

Is this true? And if so are there conditions under which it would be safe to ignore the cl->filter_cnt in htb_delete( ... ) ? 

 	
Comment 4 Russell Davies 2008-11-02 04:27:12 UTC
After rooting through the source code, I now find that this is not a bug at all.  If all filters that _refer to_ the class in question are removed first, then the class can be removed.  My original bug report is in fact nothing more than an error in procedure. 

Note You need to log in before you can comment on or make changes to this bug.