Bug 206478

Fascinating. As best I can tell, nothing in the handling of SIOCSLAGG changed between 10.1 and 10.2:

svn diff --diff-cmd=diff -x -U999999 https://svn0.us-west.freebsd.org/base/release/10.1.0/sys/net/if_lagg.c https://svn0.us-west.freebsd.org/base/release/10.2.0/sys/net/if_lagg.c

Note that there are no differences in lagg_ioctl()

> Based on our cursory code reading, below looks like the culprit in function
> lagg_ioctl().
> Please let us know if the below patch works for you, (taken with head).
>
> Thanks,
> LN
>
> Index: sys/net/if_lagg.c
> ===================================================================
> --- sys/net/if_lagg.c    (revision 292047)
> +++ sys/net/if_lagg.c    (working copy)
> @@ -1,2219 +1,2219 @@
> <snipped>
>          break;
>      case SIOCSLAGG:
>          error = priv_check(td, PRIV_NET_LAGG);
>          if (error)
>              break;
> -        if (ra->ra_proto < 1 || ra->ra_proto >= LAGG_PROTO_MAX) {
> +        if (ra->ra_proto >= LAGG_PROTO_MAX) {
>              error = EPROTONOSUPPORT;
>              break;
>          }
>
>          LAGG_WLOCK(sc);
>          lagg_proto_detach(sc);
>          LAGG_UNLOCK_ASSERT(sc);
>          lagg_proto_attach(sc, ra->ra_proto);
>          break;
>      case SIOCGLAGGOPTS:
> <snipped>

(In reply to LN from comment #2)
That portion of code appears different in 10.2-STABLE, is this perhaps the part that should be changed in that?

if (proto->ti_proto == LAGG_PROTO_NONE) {
	error = EPROTONOSUPPORT;
	break;
}

Comment 4 Marcelo Araujo 2016-01-23 17:41:47 UTC

I'm gonna take it.

This issue is seen in HOL (FreeBSD-11) also. The fix proposed by LN was related to HOL.

Tested the patch on HOL and the fix is working well.

For stable 10.2, more changes were required along with this fix.

I suggest we should take this fix to HOL.

Let me know if I can create a seperate Bug for HOL and send the patch to review in Phabricator.

Comment 6 Marcelo Araujo 2016-01-29 03:27:58 UTC

It is waiting for patch review.

https://reviews.freebsd.org/D5076

Please find fix for 10.3-STABLE.

https://reviews.freebsd.org/D5188

(Note - In this bug 'Version' is marked as 10.2-STABLE, I think it has to be 
10.3-STABLE)

As per the FreeBSD Lagg man page, laggproto none is a valid configuration option for users. 

Attached patch considers laggproto none as a valid configuration option. 
i.e 'ifconfig lagg0 laggproto none' command results into success and applies 
none protocol to lagg bundle.

Comment 8 Marcelo Araujo 2016-02-04 11:35:18 UTC

Thanks for the patch, actually on CURRENT we have the same problem and I have a patch for that already.

I'm checking on stable and soon will give you a reply.

Hi  Marcelo Araujo,

As per your patch, 'laggproto none' is not a valid configuration option.
But as per the 'lagg' man page, laggproto none is a valid configuration option. 

As per my understanding, when user applies 'ifconfig lagg0 laggproto none'
command, it should result in success and should set laggproto to 'none'.

By the way, you have proposed a fix for HOL, can we track it as a separate bug ? 
 
Thanks,
Pushkar Kothavade.

Thanks Marcelo for your feedback.

Comment 11 Marcelo Araujo 2016-02-04 11:45:42 UTC

(In reply to Pushkar Kothavade from comment #9)

Note that lagg(4) on HEAD is pretty much different from STABLE.
Did you make a try on my patch on HEAD?

Comment 12 Marcelo Araujo 2016-02-04 11:47:40 UTC

(In reply to Pushkar Kothavade from comment #9)

Sure, we should track it with two different bug reports.
Do you mind open another one for the CURRENT with the reference to that patch I have proposed? Would be nice too, if you could help me to double test that patch too.

All the best.

Hi  Marcelo, I will do the needful.

Hi  Marcelo,

I have created a separate bug (Bug 206921) for 11.0-CURRENT.
I am testing your patch on 11.0-CURRENT setup. 
Will post the test results once done. 

Thanks,
Pushkar Kothavade.

Comment 15 Marcelo Araujo 2016-02-04 12:41:13 UTC

I have the approve necessary to commit it, but I will give an extra couple of days to other people involved to take a double look on it.

Thanks for the patch!

Thanks Marcelo. 

Three reviewers have accepted the changes. 
Could you please tell me check-in process ?

Comment 17 Marcelo Araujo 2016-02-05 06:46:11 UTC

(In reply to Pushkar Kothavade from comment #16)

One of the reviewer is myself.

So, I have contacted re@ already, and we need to wait the BETA1 build finish. After that, it is likely we will get the patch in.

Best,

Comment 18 commit-hook 2016-02-25 15:34:07 UTC

A commit references this bug:

Author: araujo
Date: Thu Feb 25 15:33:55 UTC 2016
New revision: 296040
URL: https://svnweb.freebsd.org/changeset/base/296040

Log:
  MFH 295796 (based on)
  Fix regression introduced on 272446r. lagg(4) supports the protocol none,
  where it disables any traffic without disabling the lagg(4) interface itself.

  PR:		206478
  Submitted by:	Erin Clark <erin.clark.ix@gmail.com>
  Reviewed by:	rpokala, bapt
  Approved by:	re (glebius)
  Differential Revision:	https://reviews.freebsd.org/D5188

Changes:
  stable/10/sys/net/if_lagg.c

Comment 19 Marcelo Araujo 2016-02-25 15:35:14 UTC

Fixed. Thanks!