[developer] 5197-5200 Various Joyent IPF bugs/improvements

Discussion:

Robert Mustacchi via illumos-developer

2014-10-01 21:08:07 UTC

The following is a set of thee bugs/RFEs that we've made to IPF over
time at Joyent. This covers 5197-5200. These are three separate changes:

1) 5200 - Simple bug fix by Jerry Jelinek

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5200/index.html

2) 5199 - Another simple bug fix by Rob Gulewich

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5199/index.html

3) 5197/5198 - This is a rather large improvement to IPF by Rob
Gulewich. It allows the global zone to administer the ipf rule sets in
the non-global zones and additionally gives the global zone a logically
separate rule set that it can put in place for the zone/netstack. The
zone/netstack cannot itself observe this rule set which means that a
super user in the zone cannot disable it, but can still put their own
restrictions in place. For example, imagine a global zone policy where
by all netstacks had blocked port 25 access, but then the NGZ can
establish whatever it needs for its services.

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5198/index.html

And finally a webrev with everything together:

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.html

Thanks,
Robert

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Igor Kozhukhov via illumos-developer

2014-10-01 21:42:18 UTC

Permalink

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5200/usr/src/u
ts/common/inet/ipf/ip_fil_solaris.c.cdiff.html

5200

About int tmp;

We have defined int tmp', but not use variable if(ifs->_f == NULL)

Why not define it at:
! int tmp = net_hook_unregister(ifs->_f, \

5199 contain 5200

--
Best regards,
Igor Kozhukhov

On 02/10/14 01:08, "Robert Mustacchi via illumos-developer"

Post by Robert Mustacchi via illumos-developer
The following is a set of thee bugs/RFEs that we've made to IPF over
1) 5200 - Simple bug fix by Jerry Jelinek
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5200/index.ht
ml
2) 5199 - Another simple bug fix by Rob Gulewich
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5199/index.ht
ml
3) 5197/5198 - This is a rather large improvement to IPF by Rob
Gulewich. It allows the global zone to administer the ipf rule sets in
the non-global zones and additionally gives the global zone a logically
separate rule set that it can put in place for the zone/netstack. The
zone/netstack cannot itself observe this rule set which means that a
super user in the zone cannot disable it, but can still put their own
restrictions in place. For example, imagine a global zone policy where
by all netstacks had blocked port 25 access, but then the NGZ can
establish whatever it needs for its services.
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5198/index.ht
ml
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.htm
l
Thanks,
Robert
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
https://www.listbox.com/member/archive/rss/182179/21175093-0a5a4566
https://www.listbox.com/member/?&
4849
Powered by Listbox: http://www.listbox.com

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Dan McDonald via illumos-developer

2014-10-06 17:39:28 UTC

Permalink

I'm fine with this fix. Count me a a happy reviewer.

Post by Robert Mustacchi via illumos-developer
2) 5199 - Another simple bug fix by Rob Gulewich
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/5199/index.html

I'm also fine with this fix. Again, count me as a happy reviewer.

5197 & 5198 deserve their own review mail, just because of the sheer size.

Dan

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Darren Reed via illumos-developer

2014-10-16 13:17:15 UTC

Permalink

It's great to see that someone did this as this was an area that I had
always
felt didn't get treated properly and to the maximum extent. I'll try and
make
time to review this over the coming weeks.

Cheers,
Darren

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Rob Gulewich via illumos-developer

2014-10-16 17:50:04 UTC

Permalink

Post by Darren Reed via illumos-developer
It's great to see that someone did this as this was an area that I had
always
felt didn't get treated properly and to the maximum extent. I'll try and
make
time to review this over the coming weeks.

Great - thanks for taking a look!

Igor - I updated the webrev to include your suggestion:

https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.html

-- Rob

--
Rob Gulewich, Joyent (http://www.joyent.com)

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Igor Kozhukhov via illumos-developer

2014-10-16 18:22:37 UTC

Permalink

Hello All,

I can see #if SOLARIS¹ words in files.
question: can we use ILLUMOS ?

Also - I can see different checks:
#if SOLARIS

#ifdef SOLARIS

#if defined(SOLARIS)

Probably - will be better unify it to one?

Example:
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/usr/src/ut
s/common/inet/ipf/netinet/ip_fil.h.cdiff.html

I understand about:
#if defined(SOLARIS) && defined(_KERNEL)

But for other checks will be better to have '#if defined(SOLARIS)¹ or
similar, but the same for all.

I can understand about change 2012 & 2013 year, but, I think, will be
better update it to 2014?

--
Best regards,
Igor Kozhukhov

On 16/10/14 21:50, "Rob Gulewich via illumos-developer"

Post by Rob Gulewich via illumos-developer

Great - thanks for taking a look!
https://us-east.manta.joyent.com/rmustacc/public/webrevs/ipf/all/index.htm
l
-- Rob
--
Rob Gulewich, Joyent (http://www.joyent.com)
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
https://www.listbox.com/member/archive/rss/182179/21175093-0a5a4566
https://www.listbox.com/member/?&
4849
Powered by Listbox: http://www.listbox.com

Darren Reed via illumos-developer

2014-10-23 10:35:01 UTC

Permalink

Looking at the user space components only...

usr/src/cmd/ipf/lib/common/load_hash.c
ok
usr/src/cmd/ipf/lib/common/load_hashnode.c
ok
usr/src/cmd/ipf/lib/common/load_pool.c
ok
usr/src/cmd/ipf/lib/common/load_poolnode.c
ok
usr/src/cmd/ipf/tools/Makefile.tools
ok
usr/src/cmd/ipf/tools/ipf.c
ok
usr/src/cmd/ipf/tools/ipfs.c
ok
usr/src/cmd/ipf/tools/ipfstat.c
ok
usr/src/cmd/ipf/tools/ipfzone.c
ok
usr/src/cmd/ipf/tools/ipfzone.h
ok
usr/src/cmd/ipf/tools/ipmon.c
ok
usr/src/cmd/ipf/tools/ipnat.c
ok
usr/src/cmd/ipf/tools/ippool.c
ok
usr/src/man/man1m/ipf.1m
ok
usr/src/man/man1m/ipfs.1m
ok
usr/src/man/man1m/ipfstat.1m
ok
usr/src/man/man1m/ipmon.1m
ok
usr/src/man/man1m/ipnat.1m
ok
usr/src/man/man1m/ippool.1m
ok
usr/src/man/man5/ipfilter.5
215: Consider starting with "Each non-global zone in the system ...".

Also, add a simple (similar) diagram representing the global zone, GZ
Firewall
and external. This is to distinguish it from the "In-Zone" and
"GZ-Controlled"
firewalls.

An overall comment is to decide what you want to do with getopt, the
extra options and how they are parsed/printed if "SOLARIS" isn't defined
when compiled. Personally, my advice to you would be to just remove the
"#if SOLARIS" blocks that you've introduced and just concentrate on making
it work consistently for illumos throughout. At present there is a bit of a
mixture of behaviour when SOLARIS is not defined.

Cheers,
Darren

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Darren Reed via illumos-developer

2014-10-23 10:45:09 UTC

Permalink

Ach! There was something I forgot to mention.

What should the behaviour be for "-G global" when running tools in the
global zone?
Should (for example) setzonename_global() still set ipfz_gz to 1?
What about "-z global"?
Should they return an error (without calling an ioctl), do nothing or ...?

Consider that when the user is doing this, they are expecting to control
the firewall
of a "sub-zone" and so far as I know, a sub-zone cannot be called "global".

Cheers,
Darren

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com

Darren Reed via illumos-developer

2014-10-23 14:36:31 UTC

Permalink

usr/src/uts/common/inet/ipf/netinet/ipf_stack.h
ok

usr/src/uts/common/inet/ipf/netinet/ip_fil.h
ok

usr/src/uts/common/inet/ipf/ip_state.c
#5199 - the pre-patch behaviour was deliberate: state is created when
packets
that have an expectation of a reply are seen. For ICMP_ECHOREPLY it is
rather
obvious that a reply back is not expected (assume single packet rather than
a person running ping) thus it was never included in that case statement
however if that behaviour is not beneficial, I won't argue against this
change.

usr/src/uts/common/inet/ipf/ip_log.c
ok

usr/src/uts/common/inet/ipf/netinet/ipf_stack.h
ok

usr/src/uts/common/inet/ipf/ip_fil_solaris.c
Line 963: I'm not a big fan of using "-1" to represent an invalid
zone because
-1 is define'd to ALL_ZONES (sys/zone.h)

usr/src/uts/common/inet/ipf/fil.c
ok

usr/src/uts/common/inet/ipf/solaris.c
271-291 A better idea might be to use "ipf_gz" so that scripts
collecting the statistics
can more easily recognise the values. Additionally, if there are further
kstats added then
a "_gz" version of each kstat value is required rather than just having
all of the kstats
owned by a "_gz" module.

-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/21175072-86d49504
Modify Your Subscription: https://www.listbox.com/member/?member_id=21175072&id_secret=21175072-abdf7b7e
Powered by Listbox: http://www.listbox.com