Bug 206478 - Setting laggproto fails on 10.2 (SIOCSLAGG: Protocol not supported)
Summary: Setting laggproto fails on 10.2 (SIOCSLAGG: Protocol not supported)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.2-STABLE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Marcelo Araujo
URL:
Keywords: needs-qa, patch, regression
Depends on:
Blocks:
 
Reported: 2016-01-22 00:01 UTC by Erin Clark
Modified: 2016-02-25 15:35 UTC (History)
6 users (show)

See Also:
koobs: mfc-stable10?
koobs: mfc-stable9?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Erin Clark 2016-01-22 00:01:42 UTC
On FreeBSD 10.1 I can do:

ifconfig create lagg0
ifconfig lagg0 laggproto none

just fine but on 10.2 I get this when I try to do that:

ifconfig: SIOCSLAGG: Protocol not supported
Comment 1 Ravi Pokala 2016-01-22 00:27:16 UTC
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()
Comment 2 LN 2016-01-22 17:04:42 UTC
> 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>
Comment 3 Erin Clark 2016-01-22 19:26:09 UTC
(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 freebsd_committer 2016-01-23 17:41:47 UTC
I'm gonna take it.
Comment 5 Gopal 2016-01-27 10:35:36 UTC
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 freebsd_committer 2016-01-29 03:27:58 UTC
It is waiting for patch review.

https://reviews.freebsd.org/D5076
Comment 7 Pushkar Kothavade 2016-02-04 11:29:01 UTC
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 freebsd_committer 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.
Comment 9 Pushkar Kothavade 2016-02-04 11:40:05 UTC
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.
Comment 10 Pushkar Kothavade 2016-02-04 11:41:12 UTC
Thanks Marcelo for your feedback.
Comment 11 Marcelo Araujo freebsd_committer 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 freebsd_committer 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.
Comment 13 Pushkar Kothavade 2016-02-04 12:04:14 UTC
Hi  Marcelo, I will do the needful.
Comment 14 Pushkar Kothavade 2016-02-04 12:25:09 UTC
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 freebsd_committer 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!
Comment 16 Pushkar Kothavade 2016-02-05 06:39:37 UTC
Thanks Marcelo. 

Three reviewers have accepted the changes. 
Could you please tell me check-in process ?
Comment 17 Marcelo Araujo freebsd_committer 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 freebsd_committer 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 freebsd_committer 2016-02-25 15:35:14 UTC
Fixed. Thanks!