Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Jan 2022 15:53:20 -0800
From:   Martin KaFai Lau <kafai@fb.com>
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
CC:     Jakub Sitnicki <jakub@cloudflare.com>,
        Menglong Dong <menglong8.dong@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Mengen Sun <mengensun@tencent.com>, <flyingpeng@tencent.com>,
        <mungerjiang@tencent.com>, Menglong Dong <imagedong@tencent.com>
Subject: Re: [PATCH bpf-next] bpf: Add document for 'dst_port' of 'struct
 bpf_sock'
Message-ID: <20220125235320.fx775qsdtqon272v@kafai-mbp.dhcp.thefacebook.com>
References: <20220113070245.791577-1-imagedong@tencent.com>
 <87sftbobys.fsf@cloudflare.com>
 <20220125224524.fkodqvknsluihw74@kafai-mbp.dhcp.thefacebook.com>
 <CAADnVQKbYCCYjCMhEV7p1YzkAVSKvg-1VKfWVQYVL0TaESNxBQ@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAADnVQKbYCCYjCMhEV7p1YzkAVSKvg-1VKfWVQYVL0TaESNxBQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?+56KRqnTdu+J7Z7b5ufyH3BctUUBw/ZyrOnnWerHG1YIlQd2ragfh2F8BBmQ?=
 =?us-ascii?Q?AUe3y1yI6WylkCebYVB8emK1rDbwXweA/4NTeHtZiGZdHBoTKA+7Z2Pu8T4d?=
 =?us-ascii?Q?ZIc4zndXSGSTt3EDI91PsqQeexRONoCcRJitwbsb4Ghpvj+UXifu9QFbH9X4?=
 =?us-ascii?Q?6w3db2nM0D64CieKTkd/0YD9u19ta4TgufY3WWCsB4fk15+fhvE6I6B2ECmW?=
 =?us-ascii?Q?41eQQz8va9xjqRir6uW+PZOS1YIeM0AIJ4z1fAHpPQzlFB98EdwrHvSt69+B?=
 =?us-ascii?Q?TCoekJs0FvmVkgijVnh/d3BPKSozxJ8JFBrFDBy2evlJGaGRBGZYjSg2btME?=
 =?us-ascii?Q?nxCdun8H8GddC0JD0AK+3415TyxGJBPusL9VpckQ/Dd0ShnocbkwAz/AoWyd?=
 =?us-ascii?Q?jb/h9tq4cPLCLQZziyCokiRltBwhE62d4kMUgjBpMZxrfTlwnatmWrPLZTVg?=
 =?us-ascii?Q?3/eMTb039di9n57Lx9NmtVGIYD6RMgH5ID51AX0cSF9m91HtXyyHM/xjE0Mh?=
 =?us-ascii?Q?N8Yn0eK5v0nRGk+E2/Sz+MSsq7eZLNydueAirFud6r8zom9R1ht2yRlYifTP?=
 =?us-ascii?Q?ugdRFRCCdrJNZtc9YR/3KioJ+xypWvMfVUgqB0fUx0Q9Hdaur0O7/arcWkm7?=
 =?us-ascii?Q?debjfj9j9DSwk7dRVthkF862pjxq6mK7p6WwfAhg6/X0PxRR9mxToES6/MyY?=
 =?us-ascii?Q?cAfCv8+leXOQUGb5iqOuVqeosLPM9nqNSl70Ku15sjHYEBJ1agfVyVF/IKcU?=
 =?us-ascii?Q?H+RHRpBuFRZipxv05X4h9M/R2rgDhV7l2YMH1HBTG/gX1ysiLEHnuOCUNs+B?=
 =?us-ascii?Q?vNcmYDVLQ011Yg+zExa0EAsH3R/o22NbOZs5CBMXXjzJNUPrmzr+p4G+S4c1?=
 =?us-ascii?Q?4/vjvZWMVjWLm4CKiV8axrVUXbxuXwi+tcmYBQYatwwUGE38iEg2EI8pxC6q?=
 =?us-ascii?Q?y67Q5cQe68ZOKa+2jOe0PwJPmkcUxpVXrejwZPG3HaL5BGdpCxNGU7oKMRpZ?=
 =?us-ascii?Q?SbkvWyIC3lxqjc+U6ny0iD0dSlX8uC3JlMW8pCtzIN5CfqIBHKzVqBAMNBUF?=
 =?us-ascii?Q?sBJ3mMJe7hakcFV8pjOYqYn7IsUZUdeD3hnMYp9v1Wl/bP+TM8pop8NTFWiP?=
 =?us-ascii?Q?srZOs4b/kWVMDGEuBgIj5Y2NU+Ss+U8UvtAv8/vHe61qMQSSUzYNqV1vKxKM?=
 =?us-ascii?Q?lHAw0XrPz0sHueZtzJzJAfne0LOpRyyQWSsswCs+akt8Cf3u+V4d8udSg6nM?=
 =?us-ascii?Q?rEVW1BG2/DmK4c+zCGW+0em8dwRvfYz5vh83Var0XuAnfFTzLFaiwCpN3TQ4?=
 =?us-ascii?Q?V3xakF+jAuaxUlSjyPHE0MOyPkwIkqWolDGBgLc6LmnFX7hHutGFdK7ZM7vz?=
 =?us-ascii?Q?eFft/FGWF1p51GEQ47X1xM7Vhlc5PGD75zveXx3tjlT+5a2m4Uw6kT8vAxlY?=
 =?us-ascii?Q?E2KoF+h+2cNHyn1q7lVIbxNOcVjn2JmH0KzzjtGU98tb/+MfrwwwF4Xgn33B?=
 =?us-ascii?Q?vyEM7DbyAiRr3upXSMuPzrYT7OlBvP+/9IhBlFl1Rh1JmxWI+bVeVdJS1iig?=
 =?us-ascii?Q?Gc9fol1Uls37egL5+csY7Bb2K7uINuItIp02HLvy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4ae82ff7-0fa2-4ab0-b21b-08d9e05ddf49
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5016.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2022 23:53:23.4395
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wYPRfNqMF3RLfAl7qVjtC0uSDCrBnl4KZC6MSV9i9bSG3HJySODj/MNDRntSu9GA
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1436
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: xtVNz6j7RvZ6rj2OiqybuE1-0PaEKIII
X-Proofpoint-ORIG-GUID: xtVNz6j7RvZ6rj2OiqybuE1-0PaEKIII
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.816,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-01-25_06,2022-01-25_02,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 suspectscore=0
 bulkscore=0 phishscore=0 adultscore=0 spamscore=0 lowpriorityscore=0
 mlxlogscore=621 clxscore=1015 impostorscore=0 priorityscore=1501
 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2201250144
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jan 25, 2022 at 03:02:37PM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 25, 2022 at 2:45 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Tue, Jan 25, 2022 at 08:24:27PM +0100, Jakub Sitnicki wrote:
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index b0383d371b9a..891a182a749a 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -5500,7 +5500,11 @@ struct bpf_sock {
> > > >     __u32 src_ip4;
> > > >     __u32 src_ip6[4];
> > > >     __u32 src_port;         /* host byte order */
> > > > -   __u32 dst_port;         /* network byte order */
> > > > +   __u32 dst_port;         /* low 16-bits are in network byte order,
> > > > +                            * and high 16-bits are filled by 0.
> > > > +                            * So the real port in host byte order is
> > > > +                            * bpf_ntohs((__u16)dst_port).
> > > > +                            */
> > > >     __u32 dst_ip4;
> > > >     __u32 dst_ip6[4];
> > > >     __u32 state;
> > >
> > > I'm probably missing something obvious, but is there anything stopping
> > > us from splitting the field, so that dst_ports is 16-bit wide?
> > >
> > > I gave a quick check to the change below and it seems to pass verifier
> > > checks and sock_field tests.
> > >
> > > IDK, just an idea. Didn't give it a deeper thought.
> > >
> > > --8<--
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 4a2f7041ebae..344d62ccafba 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -5574,7 +5574,8 @@ struct bpf_sock {
> > >       __u32 src_ip4;
> > >       __u32 src_ip6[4];
> > >       __u32 src_port;         /* host byte order */
> > > -     __u32 dst_port;         /* network byte order */
> > > +     __u16 unused;
> > > +     __u16 dst_port;         /* network byte order */
> > This will break the existing bpf prog.
> 
> I think Jakub's idea is partially expressed:
> +       case offsetof(struct bpf_sock, dst_port):
> +               bpf_ctx_record_field_size(info, sizeof(__u16));
> +               return bpf_ctx_narrow_access_ok(off, size, sizeof(__u16));
> 
> Either 'unused' needs to be after dst_port or
> bpf_sock_is_valid_access() needs to allow offset at 'unused'
> and at 'dst_port'.
> And allow u32 access though the size is actually u16.
> Then the existing bpf progs (without recompiling) should work?
Yes, I think that should work with the existing bpf progs.
I suspect putting 'dst_port' first and then followed by 'unused'
may be easier.  That will also serve as a natural doc for the
current behavior (the value is in the lower 16 bits).

It can be extended to bpf_sk_lookup? bpf_sk_lookup can read at any
offset of these 4 bytes, so may need to read 0 during
convert_ctx_accesses?