Re: [PATCH net-next 2/2] bpf: add bpf_redirect() helper

[John Fastabend <john.fastabend@gmail.com>]

On 15-09-15 09:11 PM, Alexei Starovoitov wrote:
> On 9/15/15 8:10 PM, John Fastabend wrote:
>> Nice, I like this. But just to be sure I read this correctly this will
>> only work on the ingress qdisc for now right? To get the tx side working
>> will require a bit more care.
> 
> correct.
> For egress I'm waiting for Daniel to resubmit his preclassifier patch
> and I'll hook this skb_do_redirect() there as well.
> Other options are also possible, but preclassifier looks the best for
> this purpose, since it's lockless.
> 

Great, works for me. One other question/observation,

+int skb_do_redirect(struct sk_buff *skb)
+{

[...]

+
+	if (unlikely(!(dev->flags & IFF_UP))) {
+		kfree_skb(skb);
+		return -EINVAL;
+	}


The IFF_UP check is not needed as best I can tell, the dev_queue_xmit()
will check if the qdisc is active and the dev_forward_skb() path will
do a !netif_running check in enqueue_to_backlog() call.

Looks like you can remove the check. I would prefer to let the stack
handle this case using normal mechanisms.

I had to do a bit of tracking but netif_running check equates roughly
to your IFF_UP case via,

> 	__dev_change_flags()
> 		[...]
> 	        if ((old_flags ^ flags) & IFF_UP)
>                 	ret = ((old_flags & IFF_UP) ? __dev_close : __dev_open)(dev);
> 		
> 
> 	__dev_close()
> 		[...]
> 		__dev_close_many()
> 
> 	__dev_close_many()
> 		[...]
>               clear_bit(__LINK_STATE_START, &dev->state);

Seem reasonable? Or did you put it there to work around some specific
case I'm missing?

.John