Re: git: fcb3f813f379 - main - netinet*: remove PRC_ constants and streamline ICMP processing

In reply to: Dmitry Chagin : "Re: git: fcb3f813f379 - main - netinet*: remove PRC_ constants and streamline ICMP processing"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Randall Stewart <rrs_at_netflix.com>
Date: Tue, 04 Oct 2022 11:12:47 UTC
Dmitry:

Yeah I hit the same issue.. I did find that if you add

nooptions       IPSEC_SUPPORT

to your build it will work. Also making LINT works
so I suspect its something minor in Gleb’s re-arrangement. 
Though its like 4am right now in California so he won’t
be up for a while I suspect :)

R

> On Oct 4, 2022, at 7:10 AM, Dmitry Chagin <dchagin@heemeyer.club> wrote:
> 
> On Tue, Oct 04, 2022 at 03:57:37AM +0000, Gleb Smirnoff wrote:
>> The branch main has been updated by glebius:
>> 
>> URL: https://www.google.com/url?q=https://cgit.FreeBSD.org/src/commit/?id%3Dfcb3f813f379f544f9cd2a10d18045588da0e132&source=gmail-imap&ust=1665486631000000&usg=AOvVaw2VvUECoWSpNqOYdx0tDraU
>> 
>> commit fcb3f813f379f544f9cd2a10d18045588da0e132
>> Author:     Gleb Smirnoff <glebius@FreeBSD.org>
>> AuthorDate: 2022-10-04 03:53:04 +0000
>> Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
>> CommitDate: 2022-10-04 03:53:04 +0000
>> 
>>    netinet*: remove PRC_ constants and streamline ICMP processing
>> 
>>    In the original design of the network stack from the protocol control
>>    input method pr_ctlinput was used notify the protocols about two very
>>    different kinds of events: internal system events and receival of an
>>    ICMP messages from outside.  These events were coded with PRC_ codes.
>>    Today these methods are removed from the protosw(9) and are isolated
>>    to IPv4 and IPv6 stacks and are called only from icmp*_input().  The
>>    PRC_ codes now just create a shim layer between ICMP codes and errors
>>    or actions taken by protocols.
>> 
>>    - Change ipproto_ctlinput_t to pass just pointer to ICMP header.  This
>>      allows protocols to not deduct it from the internal IP header.
>>    - Change ip6proto_ctlinput_t to pass just struct ip6ctlparam pointer.
>>      It has all the information needed to the protocols.  In the structure,
>>      change ip6c_finaldst fields to sockaddr_in6.  The reason is that
>>      icmp6_input() already has this address wrapped in sockaddr, and the
>>      protocols want this address as sockaddr.
>>    - For UDP tunneling control input, as well as for IPSEC control input,
>>      change the prototypes to accept a transparent union of either ICMP
>>      header pointer or struct ip6ctlparam pointer.
>>    - In icmp_input() and icmp6_input() do only validation of ICMP header and
>>      count bad packets.  The translation of ICMP codes to errors/actions is
>>      done by protocols.
>>    - Provide icmp_errmap() and icmp6_errmap() as substitute to inetctlerrmap,
>>      inet6ctlerrmap arrays.
>>    - In protocol ctlinput methods either trust what icmp_errmap() recommend,
>>      or do our own logic based on the ICMP header.
>> 
>>    Differential revision:  https://www.google.com/url?q=https://reviews.freebsd.org/D36731&source=gmail-imap&ust=1665486631000000&usg=AOvVaw1JtS6e8srYG7hprLpt68zn
>> ---
>> sys/netinet/icmp6.h          |   1 +
>> sys/netinet/in_var.h         |   2 -
>> sys/netinet/ip_icmp.c        | 106 +++++++++++++++++++----------------
>> sys/netinet/ip_icmp.h        |   1 +
>> sys/netinet/ip_input.c       |  17 ------
>> sys/netinet/ip_var.h         |   3 +-
>> sys/netinet/raw_ip.c         |  11 +---
>> sys/netinet/sctp_usrreq.c    |  10 ++--
>> sys/netinet/sctp_var.h       |   2 +-
>> sys/netinet/sctputil.c       |  13 ++---
>> sys/netinet/tcp_subr.c       | 128 +++++++++++++++++++++++++------------------
>> sys/netinet/udp_usrreq.c     |  24 ++++----
>> sys/netinet/udp_var.h        |   6 +-
>> sys/netinet6/icmp6.c         | 100 ++++++++++++++++++---------------
>> sys/netinet6/in6_pcb.c       |  26 +--------
>> sys/netinet6/in6_var.h       |   1 -
>> sys/netinet6/ip6_input.c     |  20 -------
>> sys/netinet6/ip6_var.h       |  11 ++--
>> sys/netinet6/raw_ip6.c       |  31 ++---------
>> sys/netinet6/sctp6_usrreq.c  |   8 +--
>> sys/netinet6/udp6_usrreq.c   |  26 ++++-----
>> sys/netipsec/ipsec_input.c   |  24 ++++----
>> sys/netipsec/ipsec_support.h |  17 ++++--
>> sys/sys/protosw.h            |  43 ---------------
>> 24 files changed, 274 insertions(+), 357 deletions(-)
>> 
>> diff --git a/sys/netinet/icmp6.h b/sys/netinet/icmp6.h
>> index 9628c0957c4a..7429b8173b6a 100644
>> --- a/sys/netinet/icmp6.h
>> +++ b/sys/netinet/icmp6.h
>> @@ -701,6 +701,7 @@ struct	rttimer;
>> struct	in6_multi;
>> # endif
>> void	icmp6_paramerror(struct mbuf *, int);
>> +int	icmp6_errmap(const struct icmp6_hdr *);
>> void	icmp6_error(struct mbuf *, int, int, int);
>> void	icmp6_error2(struct mbuf *, int, int, int, struct ifnet *);
>> int	icmp6_input(struct mbuf **, int *, int);
>> diff --git a/sys/netinet/in_var.h b/sys/netinet/in_var.h
>> index 40e1c1a23c40..c4cfeea66ba8 100644
>> --- a/sys/netinet/in_var.h
>> +++ b/sys/netinet/in_var.h
>> @@ -100,8 +100,6 @@ struct in_ifaddr {
>> #define IN_LNAOF(in, ifa) \
>> 	((ntohl((in).s_addr) & ~((struct in_ifaddr *)(ifa)->ia_subnetmask))
>> 
>> -extern	u_char	inetctlerrmap[];
>> -
>> #define LLTABLE(ifp)	\
>> 	((struct in_ifinfo *)(ifp)->if_afdata[AF_INET])->ii_llt
>> /*
>> diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
>> index f0cc703c2757..fdde24fd94be 100644
>> --- a/sys/netinet/ip_icmp.c
>> +++ b/sys/netinet/ip_icmp.c
>> @@ -403,6 +403,55 @@ freeit:
>> 	m_freem(n);
>> }
>> 
>> +int
>> +icmp_errmap(const struct icmp *icp)
>> +{
>> +
>> +	switch (icp->icmp_type) {
>> +	case ICMP_UNREACH:
>> +		switch (icp->icmp_code) {
>> +		case ICMP_UNREACH_NET:
>> +		case ICMP_UNREACH_HOST:
>> +		case ICMP_UNREACH_SRCFAIL:
>> +		case ICMP_UNREACH_NET_UNKNOWN:
>> +		case ICMP_UNREACH_HOST_UNKNOWN:
>> +		case ICMP_UNREACH_ISOLATED:
>> +		case ICMP_UNREACH_TOSNET:
>> +		case ICMP_UNREACH_TOSHOST:
>> +		case ICMP_UNREACH_HOST_PRECEDENCE:
>> +		case ICMP_UNREACH_PRECEDENCE_CUTOFF:
>> +			return (EHOSTUNREACH);
>> +		case ICMP_UNREACH_NEEDFRAG:
>> +			return (EMSGSIZE);
>> +		case ICMP_UNREACH_PROTOCOL:
>> +		case ICMP_UNREACH_PORT:
>> +		case ICMP_UNREACH_NET_PROHIB:
>> +		case ICMP_UNREACH_HOST_PROHIB:
>> +		case ICMP_UNREACH_FILTER_PROHIB:
>> +			return (ECONNREFUSED);
>> +		default:
>> +			return (0);
>> +		}
>> +	case ICMP_TIMXCEED:
>> +		switch (icp->icmp_code) {
>> +		case ICMP_TIMXCEED_INTRANS:
>> +			return (EHOSTUNREACH);
>> +		default:
>> +			return (0);
>> +		}
>> +	case ICMP_PARAMPROB:
>> +		switch (icp->icmp_code) {
>> +		case ICMP_PARAMPROB_ERRATPTR:
>> +		case ICMP_PARAMPROB_OPTABSENT:
>> +			return (ENOPROTOOPT);
>> +		default:
>> +			return (0);
>> +		}
>> +	default:
>> +		return (0);
>> +	}
>> +}
>> +
>> /*
>>  * Process a received ICMP message.
>>  */
>> @@ -484,56 +533,21 @@ icmp_input(struct mbuf **mp, int *offp, int proto)
>> 	code = icp->icmp_code;
>> 	switch (icp->icmp_type) {
>> 	case ICMP_UNREACH:
>> -		switch (code) {
>> -			case ICMP_UNREACH_NET:
>> -			case ICMP_UNREACH_HOST:
>> -			case ICMP_UNREACH_SRCFAIL:
>> -			case ICMP_UNREACH_NET_UNKNOWN:
>> -			case ICMP_UNREACH_HOST_UNKNOWN:
>> -			case ICMP_UNREACH_ISOLATED:
>> -			case ICMP_UNREACH_TOSNET:
>> -			case ICMP_UNREACH_TOSHOST:
>> -			case ICMP_UNREACH_HOST_PRECEDENCE:
>> -			case ICMP_UNREACH_PRECEDENCE_CUTOFF:
>> -				code = PRC_UNREACH_NET;
>> -				break;
>> -
>> -			case ICMP_UNREACH_NEEDFRAG:
>> -				code = PRC_MSGSIZE;
>> -				break;
>> -
>> -			/*
>> -			 * RFC 1122, Sections 3.2.2.1 and 4.2.3.9.
>> -			 * Treat subcodes 2,3 as immediate RST
>> -			 */
>> -			case ICMP_UNREACH_PROTOCOL:
>> -				code = PRC_UNREACH_PROTOCOL;
>> -				break;
>> -			case ICMP_UNREACH_PORT:
>> -				code = PRC_UNREACH_PORT;
>> -				break;
>> -
>> -			case ICMP_UNREACH_NET_PROHIB:
>> -			case ICMP_UNREACH_HOST_PROHIB:
>> -			case ICMP_UNREACH_FILTER_PROHIB:
>> -				code = PRC_UNREACH_ADMIN_PROHIB;
>> -				break;
>> -
>> -			default:
>> -				goto badcode;
>> -		}
>> -		goto deliver;
>> +		if (code > ICMP_UNREACH_PRECEDENCE_CUTOFF)
>> +			goto badcode;
>> +		else
>> +			goto deliver;
>> 
>> 	case ICMP_TIMXCEED:
>> -		if (code > 1)
>> +		if (code > ICMP_TIMXCEED_REASS)
>> 			goto badcode;
>> -		code += PRC_TIMXCEED_INTRANS;
>> -		goto deliver;
>> +		else
>> +			goto deliver;
>> 
>> 	case ICMP_PARAMPROB:
>> -		if (code > 1)
>> +		if (code > ICMP_PARAMPROB_LENGTH)
>> 			goto badcode;
>> -		code = PRC_PARAMPROB;
>> +
>> 	deliver:
>> 		/*
>> 		 * Problem with datagram; advise higher level routines.
>> @@ -553,7 +567,6 @@ icmp_input(struct mbuf **mp, int *offp, int proto)
>> 		if (icmpprintfs)
>> 			printf("deliver to protocol %d\n", icp->icmp_ip.ip_p);
>> #endif
>> -		icmpsrc.sin_addr = icp->icmp_ip.ip_dst;
>> 		/*
>> 		 * XXX if the packet contains [IPv4 AH TCP], we can't make a
>> 		 * notification to TCP layer.
>> @@ -576,8 +589,7 @@ icmp_input(struct mbuf **mp, int *offp, int proto)
>> 		 *   ICMP_ADVLENPREF. See its definition in ip_icmp.h.
>> 		 */
>> 		if (ip_ctlprotox[icp->icmp_ip.ip_p] != NULL)
>> -			ip_ctlprotox[icp->icmp_ip.ip_p](code, &icmpsrc,
>> -			    &icp->icmp_ip);
>> +			ip_ctlprotox[icp->icmp_ip.ip_p](icp);
>> 		break;
>> 
>> 	badcode:
>> diff --git a/sys/netinet/ip_icmp.h b/sys/netinet/ip_icmp.h
>> index 0303a09509c7..fefece665a00 100644
>> --- a/sys/netinet/ip_icmp.h
>> +++ b/sys/netinet/ip_icmp.h
>> @@ -216,6 +216,7 @@ struct icmp {
>> 	(type) == ICMP_MASKREQ || (type) == ICMP_MASKREPLY)
>> 
>> #ifdef _KERNEL
>> +int	icmp_errmap(const struct icmp *);
>> void	icmp_error(struct mbuf *, int, int, uint32_t, int);
>> int	icmp_input(struct mbuf **, int *, int);
>> int	ip_next_mtu(int, int);
>> diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
>> index 145c4464b855..88fd4f5e4def 100644
>> --- a/sys/netinet/ip_input.c
>> +++ b/sys/netinet/ip_input.c
>> @@ -873,23 +873,6 @@ ipproto_unregister(uint8_t proto)
>> 		return (ENOENT);
>> }
>> 
>> -/* (x) - issued by icmp_input() */
>> -u_char inetctlerrmap[PRC_NCMDS] = {
>> -	[PRC_MSGSIZE] = EMSGSIZE,			/* (x) */
>> -	[PRC_HOSTDEAD] = EHOSTDOWN,
>> -	[PRC_HOSTUNREACH] = EHOSTUNREACH,
>> -	[PRC_UNREACH_NET] = EHOSTUNREACH,		/* (x) */
>> -	[PRC_UNREACH_HOST] = EHOSTUNREACH,
>> -	[PRC_UNREACH_PROTOCOL] = ECONNREFUSED,		/* (x) */
>> -	[PRC_UNREACH_PORT] = ECONNREFUSED,		/* (x) */
>> -	[12] = EMSGSIZE,
>> -	[PRC_UNREACH_SRCFAIL] = EHOSTUNREACH,
>> -	[PRC_TIMXCEED_INTRANS] = EHOSTUNREACH,		/* (x) */
>> -	[PRC_TIMXCEED_REASS] = 0,			/* (x) */
>> -	[PRC_PARAMPROB] = ENOPROTOOPT,			/* (x) */
>> -	[PRC_UNREACH_ADMIN_PROHIB] = ECONNREFUSED,	/* (x) */
>> -};
>> -
>> /*
>>  * Forward a packet.  If some error occurs return the sender
>>  * an icmp packet.  Note we can't always generate a meaningful
>> diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
>> index 070c82677150..0a2d915b12b3 100644
>> --- a/sys/netinet/ip_var.h
>> +++ b/sys/netinet/ip_var.h
>> @@ -238,7 +238,8 @@ extern void	(*ip_rsvp_force_done)(struct socket *);
>> extern int	(*rsvp_input_p)(struct mbuf **, int *, int);
>> 
>> typedef int	ipproto_input_t(struct mbuf **, int *, int);
>> -typedef void	ipproto_ctlinput_t(int, struct sockaddr_in *, struct ip *);
>> +struct icmp;
>> +typedef void	ipproto_ctlinput_t(struct icmp *);
>> int	ipproto_register(uint8_t, ipproto_input_t, ipproto_ctlinput_t);
>> int	ipproto_unregister(uint8_t);
>> #define	IPPROTO_REGISTER(prot, input, ctl)	do {			\
>> diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
>> index fb692e0822cf..2065b47883bb 100644
>> --- a/sys/netinet/raw_ip.c
>> +++ b/sys/netinet/raw_ip.c
>> @@ -804,17 +804,12 @@ rip_ctloutput(struct socket *so, struct sockopt *sopt)
>> }
>> 
>> void
>> -rip_ctlinput(int cmd, struct sockaddr_in *sin, struct ip *ip)
>> +rip_ctlinput(struct icmp *icmp)
>> {
>> -
>> -	switch (cmd) {
>> #if defined(IPSEC) || defined(IPSEC_SUPPORT)
>> -	case PRC_MSGSIZE:
>> -		if (IPSEC_ENABLED(ipv4))
>> -			IPSEC_CTLINPUT(ipv4, cmd, (struct sockaddr *)sin, ip);
>> -		break;
>> +	if (IPSEC_ENABLED(ipv4))
>> +		IPSEC_CTLINPUT(ipv4, icmp);
>> #endif
> 
> hi, 
> 
> me/dchagin/freebsd/sys/netinet/raw_ip.c:811:3: error: too few arguments
> to function call, expected 4, have 2
>                IPSEC_CTLINPUT(ipv4, icmp);
> 
> /home/dchagin/freebsd/sys/netipsec/ipsec_support.h:222:61: note:
> expanded from macro 'IPSEC_CTLINPUT'
>    ipsec_kmod_ctlinput(proto ## _ipsec_support, __VA_ARGS__)
> 
> 
> /home/dchagin/freebsd/sys/netipsec/ipsec_support.h:196:5: note:
> 'ipsec_kmod_ctlinput' declared here
> int ipsec_kmod_ctlinput(struct ipsec_support * const, int,
> 
> 
> 1 error generated.
> *** [raw_ip.o] Error code 1
> 
> 

------
Randall Stewart
rrs@netflix.com