act_mirred: don't go back.

don't go back.

don't go back.
----
net/sched/act_mirred.c | 24 +++++++++++++-----------

1 file changed, 13 insertions(+), 11 deletions(-)

index b9aaab4..e1d8e2c 100644
@@ -160,17 +160,7 @@ static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
 		if (net_ratelimit())
 			printk("mirred to Houston: device %s is gone!\n",
 			       dev->name);
-bad_mirred:
-		if (skb2 != NULL)
-			kfree_skb(skb2);
-		m->tcf_qstats.overlimits++;
-		m->tcf_bstats.bytes += qdisc_pkt_len(skb);
-		m->tcf_bstats.packets++;
-		spin_unlock(&m->tcf_lock);
-		/* should we be asking for packet to be dropped?
-		 * may make sense for redirect case only
-		*/
-		return TC_ACT_SHOT;
+		goto bad_mirred;
 	}
 
 	skb2 = skb_act_clone(skb, GFP_ATOMIC);

@@ -199,6 +189,18 @@ bad_mirred:
 	dev_queue_xmit(skb2);
 	spin_unlock(&m->tcf_lock);
 	return m->tcf_action;
+
+bad_mirred:
+	if (skb2 != NULL)
+		kfree_skb(skb2);
+	m->tcf_qstats.overlimits++;
+	m->tcf_bstats.bytes += qdisc_pkt_len(skb);
+	m->tcf_bstats.packets++;
+	spin_unlock(&m->tcf_lock);
+	/* should we be asking for packet to be dropped?
+	 * may make sense for redirect case only
+	 */
+	return TC_ACT_SHOT;
 }
 
 static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind, int ref)

I didnt follow the motivation for the patch. Is it a bug fix for some setup? I may be too jet-lagged and i am missing the functional difference vs what was already there.

cheers,
jamal

code cleanup! :)

Regards，
Changli Gao([email protected])

I dont really see it as a cleanup to be honest. The code is not less ugly even after the change;->

cheers,
jamal

Maybe I should remove goto, I'll try later.

Regards，
Changli Gao([email protected])

static int tcf_mirred(struct sk_buff *skb, struct tc_action *a,
struct tcf_result *res)

{
        struct net_device *dev;
        struct sk_buff *skb2;
        u32 at;
        struct tcf_mirred *m = a->priv;
        int retval, err = 1;

        spin_lock(&m->tcf_lock);
        m->tcf_tm.lastuse = jiffies;
        if (m->tcfm_eaction != TCA_EGRESS_MIRROR &&
            m->tcfm_eaction != TCA_EGRESS_REDIR) {
                if (net_ratelimit())
                        printk("tcf_mirred unknown action %d\n",
                               m->tcfm_eaction);
                goto out;
        }

        dev = m->tcfm_dev;
        if (!(dev->flags&IFF_UP) ) {
                if (net_ratelimit())
                        printk("mirred to Houston: device %s is gone!\n",
                               dev->name);
                goto out;
        }

        skb2 = skb_act_clone(skb, GFP_ATOMIC);
        if (skb2 == NULL)
                goto out;

        m->tcf_bstats.bytes += qdisc_pkt_len(skb2);
        m->tcf_bstats.packets++;
        at = G_TC_AT(skb->tc_verd);
        if (!(at & AT_EGRESS)) {
                if (m->tcfm_ok_push)
                        skb_push(skb2, skb2->dev->hard_header_len);
        }

        /* mirror is always swallowed */
        if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
                skb2->tc_verd = SET_TC_FROM(skb2->tc_verd, at);

        skb2->dev = dev;
        skb2->iif = skb->dev->ifindex;
        dev_queue_xmit(skb2);
        err = 0;

out:

        if (err) {
                m->tcf_qstats.overlimits++;
                m->tcf_bstats.bytes += qdisc_pkt_len(skb);
                m->tcf_bstats.packets++;
                /* should we be asking for packet to be dropped?
                 * may make sense for redirect case only
                 */
                retval = TC_ACT_SHOT;
        } else {
                retval = m->tcf_action;
        }
        spin_unlock(&m->tcf_lock);

        return retval;
}

How about this version.

1. move skb_act_clone() after all the necessary checks, and it can eliminate unnecessary skb_act_clone() if tcfm_eaction isn't correct.
2. there is one exit of the critical section.
3. jump forward instead of backward.

Regards，
Changli Gao([email protected])

I apologize - I am still not convinced this is a cleanup and i can already see holes you are introducing (example not freeing skb etc). You are putting me in a dilemma of not wanting to discourage you but at the same time not seeing this as a useful change to be made. Can we let this one slide?

cheers,
jamal

On Mon, 2009-11-09 at 20:33 +0800, Changli Gao wrote:

[..]

Where? After skb2 is allocated, there won't be any failure any more.

It's just OK. When using tc, I also found act_mirred doesn't support ingress, then I realized that there isn't any difference between ingress and egress, as it depends on its parent. However I do think it is confused, when it prints:
filter parent ffff: protocol ip pref 49152 basic handle 0x1
action order 1: mirred (Egress Redirect to device ifb0) stolen index 5 ref 1 bind 1.
And the TODO note still is in the source code of act_mirred, it do make me wonder for a while!

Regards，
Changli Gao([email protected])

Hi,

Sorry, yes, your change does look like an improvement. If you can test a little more - please add my sign-off and submit.

Well, you know if you have the energy - doing socket redirect would be very interesting indeed. Ingress is another beast, it requires some thinking and a lot more testing depending on the devices because of possible loops.

cheers,
jamal

OK. I'll do some test first, then send it out for review.

The current kernel and iproute2 support ingress in another way, that is attaching the filter to the corresponding ingress qdisc. refter to
http://www.linuxfoundation.org/en/Net:IFB . The loops is avoided in netif_receive_skb() and with TTL mechanism. Do you have more documentation or examples about socket redirect? Sorry, I don't know what it is used to do. Is it sth. like ipt_REDIRECT?

Regards，
Changli Gao([email protected])

that looks like something i wrote ;->

I open a socket and I install a filter to mirror or redirect to this socket. It is possible to do it today if you use tun device; thats why i have never been motivated to do it.

cheers,
jamal

PF_PACKET can mirror to socket. And with the help of ingress polices, it can emulate redirection too. Am I right?

Regards，
Changli Gao([email protected])