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;
Subject: Re: [PATCH v2 bpf-next 4/7] printk: add type-printing %pT format
 specifier which uses BTF
To:     Alan Maguire <alan.maguire@oracle.com>
CC:     <ast@kernel.org>, <daniel@iogearbox.net>, <bpf@vger.kernel.org>,
        <joe@perches.com>, <linux@rasmusvillemoes.dk>,
        <arnaldo.melo@gmail.com>, <kafai@fb.com>, <songliubraving@fb.com>,
        <andriin@fb.com>, <john.fastabend@gmail.com>,
        <kpsingh@chromium.org>, <linux-kernel@vger.kernel.org>,
        <netdev@vger.kernel.org>
References: <1589263005-7887-1-git-send-email-alan.maguire@oracle.com>
 <1589263005-7887-5-git-send-email-alan.maguire@oracle.com>
 <51ac81f5-3d74-e394-3183-5d8366e2ddcc@fb.com>
 <alpine.LRH.2.21.2005142330430.24127@localhost>
From:   Yonghong Song <yhs@fb.com>
Message-ID: <f067bbb7-ef0b-4809-3fa8-1666d03bcf5f@fb.com>
Date:   Thu, 14 May 2020 17:39:55 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.8.0
In-Reply-To: <alpine.LRH.2.21.2005142330430.24127@localhost>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from dasneha-mbp.dhcp.thefacebook.com (2620:10d:c090:400::5:1b64) by BYAPR01CA0036.prod.exchangelabs.com (2603:10b6:a02:80::49) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3000.25 via Frontend Transport; Fri, 15 May 2020 00:40:33 +0000
X-Originating-IP: [2620:10d:c090:400::5:1b64]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: d7876a54-35a5-4986-2b13-08d7f8689477
X-MS-TrafficTypeDiagnostic: BYAPR15MB2453:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BYAPR15MB24536FBB180FAADAE1D5F788D3BD0@BYAPR15MB2453.namprd15.prod.outlook.com>
X-FB-Source: Internal
X-MS-Oob-TLC-OOBClassifiers: OLM:10000;
X-Forefront-PRVS: 04041A2886
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: KUhvVAgCUKZWszX2r+QWo0KqcC0r+6BzxCKxL5NEEKVoqus+r4NDLWMHFD4oDWLzJcQgfVjb9666yPY7TfWwVHhRxRjeGmNe2HXO0WAh74ALeFFLNEv2GReSCX/POsmNGUaXyg9W1zjq9mGw6U4ZWjiSqiAar4rXDjfGt8kngeqErqQokjqH0qIhyW8ctmSAENVICMvjoxMQW5qkDYUgEAuvWtP0Krz74Zj2H7qYtSTlpd3eZn2IAlSXO+BEN9PvbP0sYuxi9ZeijxAJrww3FH8/HjVhkso1MYegHHQ1+g7JEOZBBT8RMx0l19w1arUECVqDS9lJWah79NUj6miGEBOOChsl/UUfJAOyeUGKBw10c0c5u95GAYGGaqDhH/L1HgOA+DthOpCGWTZkGCJ476eh6AkXiYp2j0a4KQVCg6addRc2Z9EpbqGHpPSecfKcrCQlnIAhA8r2rymipJVVs3nSwVwNeOg+a1jPjEKyfgXM0aBZmtV36eSJJWRsmy4Z
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR15MB4088.namprd15.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(136003)(396003)(376002)(366004)(39860400002)(346002)(186003)(31686004)(66946007)(36756003)(66476007)(4326008)(66556008)(16526019)(6512007)(8676002)(6666004)(8936002)(6916009)(316002)(2616005)(53546011)(52116002)(6506007)(6486002)(2906002)(7416002)(5660300002)(86362001)(31696002)(478600001)(43740500002);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: aDFC0dNHGJQKgrJ4HTrCaR0NBazf6ROGKTT3IpMznJrqYhrshtYYZYvhohvFj6uNG3/jJVoddtwQyR3uJ2fxz8KksguB1wuUPWEhMZnTlhHyX7lm4/zG/UUpEPhwVl0w0vOIeu7lr757Ylpu0RvdVAOPydyjiqIB0ioIlCVKNbBfQmRDNWe+QOzOJnpQEL+kWN9Fnmp40X//iiUE0jZOA6ZrTcUvKTXsJ4l5EVs+Amr74FAcp1q3M4lxBTbQCZqJfq/yvwJR+Y3uJywm1oDWIIIxRiYyMHJmR+Ck9UZDzBpyj1K1B+hLe/o8jzwwuS8DOWatKmx+aE50UI/AnYbE+gX88LPP7om6My3HZzWAWLT3ys+BP+vbXobns7zRGNbMzsCB5cebK/+LEx+Dzbci+jeWOm3lE5Y2AZTgi5TIG87mu7WR1F+LWfF2iEBFrj4+32RmSK7RtUFm9JgNnas8rVo+r6/IyUwPepbCH3wn8PbN2Ljcxy6GvGsiJVV9S/eD/12zPgSDYVMgF0nutyLzPQ==
X-MS-Exchange-CrossTenant-Network-Message-Id: d7876a54-35a5-4986-2b13-08d7f8689477
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2020 00:40:35.0797
 (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: Fb1GtQQRNdU5fmteuMRaSYp5AvXcOu/3XJLsyhoko0OyV2lhEQTY7iIv6qgWOUzr
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB2453
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.676
 definitions=2020-05-14_08:2020-05-14,2020-05-14 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 bulkscore=0
 malwarescore=0 impostorscore=0 suspectscore=0 cotscore=-2147483648
 priorityscore=1501 spamscore=0 adultscore=0 mlxscore=0 phishscore=0
 mlxlogscore=999 lowpriorityscore=0 clxscore=1015 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-2004280000
 definitions=main-2005150003
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 5/14/20 3:37 PM, Alan Maguire wrote:
> 
> 
> On Wed, 13 May 2020, Yonghong Song wrote:
> 
>>
>>
>> On 5/11/20 10:56 PM, Alan Maguire wrote:
>>> printk supports multiple pointer object type specifiers (printing
>>> netdev features etc).  Extend this support using BTF to cover
>>> arbitrary types.  "%pT" specifies the typed format, and the pointer
>>> argument is a "struct btf_ptr *" where struct btf_ptr is as follows:
>>>
>>> struct btf_ptr {
>>>   void *ptr;
>>>   const char *type;
>>>   u32 id;
>>> };
>>>
>>> Either the "type" string ("struct sk_buff") or the BTF "id" can be
>>> used to identify the type to use in displaying the associated "ptr"
>>> value.  A convenience function to create and point at the struct
>>> is provided:
>>>
>>>   printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct sk_buff));
>>>
>>> When invoked, BTF information is used to traverse the sk_buff *
>>> and display it.  Support is present for structs, unions, enums,
>>> typedefs and core types (though in the latter case there's not
>>> much value in using this feature of course).
>>>
>>> Default output is indented, but compact output can be specified
>>> via the 'c' option.  Type names/member values can be suppressed
>>> using the 'N' option.  Zero values are not displayed by default
>>> but can be using the '0' option.  Pointer values are obfuscated
>>> unless the 'x' option is specified.  As an example:
>>>
>>>     struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>>>     pr_info("%pT", BTF_PTR_TYPE(skb, struct sk_buff));
>>>
>>> ...gives us:
>>>
>>> (struct sk_buff){
>>>    .transport_header = (__u16)65535,
>>>    .mac_header = (__u16)65535,
>>>    .end = (sk_buff_data_t)192,
>>>    .head = (unsigned char *)000000006b71155a,
>>>    .data = (unsigned char *)000000006b71155a,
>>>    .truesize = (unsigned int)768,
>>>    .users = (refcount_t){
>>>     .refs = (atomic_t){
>>>     .counter = (int)1,
>>>    },
>>>    },
>>>    .extensions = (struct skb_ext *)00000000f486a130,
>>> }
>>>
>>> printk output is truncated at 1024 bytes.  For cases where overflow
>>> is likely, the compact/no type names display modes may be used.
>>>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>    Documentation/core-api/printk-formats.rst |  15 ++++
>>>    include/linux/btf.h                       |   3 +-
>>>    include/linux/printk.h                    |  16 +++++
>>>    lib/Kconfig                               |  16 +++++
>>>    lib/vsprintf.c                            | 113
>>>    ++++++++++++++++++++++++++++++
>>>    5 files changed, 162 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/core-api/printk-formats.rst
>>> b/Documentation/core-api/printk-formats.rst
>>> index 8ebe46b1..5c66097 100644
>>> --- a/Documentation/core-api/printk-formats.rst
>>> +++ b/Documentation/core-api/printk-formats.rst
>>> @@ -545,6 +545,21 @@ For printing netdev_features_t.
>>>    
>>>    Passed by reference.
>>>    
>>> +BTF-based printing of pointer data
>>> +----------------------------------
>>> +If '%pT' is specified, use the struct btf_ptr * along with kernel vmlinux
>>> +BPF Type Format (BTF) to show the typed data.  For example, specifying
>>> +
>>> +	printk(KERN_INFO "%pT", BTF_PTR_TYPE(skb, struct_sk_buff));
>>> +
>>> +will utilize BTF information to traverse the struct sk_buff * and display
>>> it.
>>> +
>>> +Supported modifers are
>>> + 'c' compact output (no indentation, newlines etc)
>>> + 'N' do not show type names
>>> + 'x' show raw pointers (no obfuscation)
>>> + '0' show zero-valued data (it is not shown by default)
>>> +
>>>    Thanks
>>>    ======
>>>    
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index d571125..7b585ab 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -169,10 +169,11 @@ static inline const struct btf_member
>>> *btf_type_member(const struct btf_type *t)
>>>    	return (const struct btf_member *)(t + 1);
>>>    }
>>>    
>>> +struct btf *btf_parse_vmlinux(void);
>>> +
>>>    #ifdef CONFIG_BPF_SYSCALL
>>>    const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>>>    const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>>> -struct btf *btf_parse_vmlinux(void);
>>>    struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>>>    #else
>>>    static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
>>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>>> index fcde0772..3c3ea53 100644
>>> --- a/include/linux/printk.h
>>> +++ b/include/linux/printk.h
>>> @@ -528,4 +528,20 @@ static inline void print_hex_dump_debug(const char
>>> *prefix_str, int prefix_type,
>>>    #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
>>>     print_hex_dump_debug(prefix_str, prefix_type, 16, 1, buf, len, true)
>>>    +/**
>>> + * struct btf_ptr is used for %pT (typed pointer) display; the
>>> + * additional type string/BTF id are used to render the pointer
>>> + * data as the appropriate type.
>>> + */
>>> +struct btf_ptr {
>>> +	void *ptr;
>>> +	const char *type;
>>> +	u32 id;
>>> +};
>>> +
>>> +#define	BTF_PTR_TYPE(ptrval, typeval) \
>>> +	(&((struct btf_ptr){.ptr = ptrval, .type = #typeval}))
>>> +
>>> +#define BTF_PTR_ID(ptrval, idval) \
>>> +	(&((struct btf_ptr){.ptr = ptrval, .id = idval}))
>>>    #endif
>> [...]
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 7c488a1..f9276f8 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -43,6 +43,7 @@
>>>    #ifdef CONFIG_BLOCK
>>>    #include <linux/blkdev.h>
>>>    #endif
>>> +#include <linux/btf.h>
>>>    
>>>    #include "../mm/internal.h"	/* For the trace_print_flags arrays */
>>>    
>>> @@ -2059,6 +2060,103 @@ char *fwnode_string(char *buf, char *end, struct
>>> fwnode_handle *fwnode,
>>>    	return widen_string(buf, buf - buf_start, end, spec);
>>>    }
>>>    
>>> +#if IS_ENABLED(CONFIG_BTF_PRINTF)
>>> +#define btf_modifier_flag(c)	(c == 'c' ? BTF_SHOW_COMPACT :	\
>>> +				 c == 'N' ? BTF_SHOW_NONAME :	\
>>> +				 c == 'x' ? BTF_SHOW_PTR_RAW :	\
>>> +				 c == '0' ? BTF_SHOW_ZERO : 0)
>>> +
>>> +static noinline_for_stack
>>> +char *btf_string(char *buf, char *end, void *ptr, struct printf_spec spec,
>>> +		 const char *fmt)
>>> +{
>>> +	struct btf_ptr *bp = (struct btf_ptr *)ptr;
>>> +	u8 btf_kind = BTF_KIND_TYPEDEF;
>>> +	const struct btf_type *t;
>>> +	const struct btf *btf;
>>> +	char *buf_start = buf;
>>> +	const char *btf_type;
>>> +	u64 flags = 0, mod;
>>> +	s32 btf_id;
>>> +
>>> +	if (check_pointer(&buf, end, ptr, spec))
>>> +		return buf;
>>> +
>>> +	if (check_pointer(&buf, end, bp->ptr, spec))
>>> +		return buf;
>>> +
>>> +	while (isalnum(*fmt)) {
>>> +		mod = btf_modifier_flag(*fmt);
>>> +		if (!mod)
>>> +			break;
>>> +		flags |= mod;
>>> +		fmt++;
>>> +	}
>>> +
>>> +	btf = bpf_get_btf_vmlinux();
>>> +	if (IS_ERR_OR_NULL(btf))
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	if (bp->type != NULL) {
>>> +		btf_type = bp->type;
>>> +
>>> +		if (strncmp(bp->type, "struct ", strlen("struct ")) == 0) {
>>> +			btf_kind = BTF_KIND_STRUCT;
>>> +			btf_type += strlen("struct ");
>>> +		} else if (strncmp(btf_type, "union ", strlen("union ")) == 0)
>>> {
>>> +			btf_kind = BTF_KIND_UNION;
>>> +			btf_type += strlen("union ");
>>> +		} else if (strncmp(btf_type, "enum ", strlen("enum ")) == 0) {
>>> +			btf_kind = BTF_KIND_ENUM;
>>> +			btf_type += strlen("enum ");
>>> +		}
>>
>> I think typedef should be supported here.
>> In kernel, we have some structure directly defined as typedef's.
>> A lot of internal int types also typedefs, like u32, atomic_t,
>> possible_net_t, etc.
>>
>> A type name without prefix "struct", "union", "enum" can be
>> treated as a typedef first.
>>
> 
> That's how the code works today; we start with a typedef assumption.
> See the comment below starting "Assume type specified is a typedef";
> we initialize btf_kind to be a typedef above; it's only changed
> to an BTF_KIND_INT if we find a struct/enum/union prefix or if lookup
> using the typedef kind fails. I should probably make this clearer
> though (move the comment up maybe?). Thanks for taking a look!

Thanks for explanation. I missed it. Move the comments up about what to 
support explicitly will be good.

> 
>> If the type name is not a typedef, it is then compared to a limited
>> number of C basic int types like "char", "unsigned char", "short",
>> "unsigned short", ...
>>
>>> +
>>> +		if (strlen(btf_type) == 0)
>>> +			return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +		/*
>>> +		 * Assume type specified is a typedef as there's not much
>>> +		 * benefit in specifying int types other than wasting time
>>> +		 * on BTF lookups; we optimize for the most useful path.
>>> +		 *
>>> +		 * Fall back to BTF_KIND_INT if this fails.
>>> +		 */
>>> +		btf_id = btf_find_by_name_kind(btf, btf_type, btf_kind);
>>> +		if (btf_id < 0)
>>> +			btf_id = btf_find_by_name_kind(btf, btf_type,
>>> +						       BTF_KIND_INT);
>>> +	} else if (bp->id > 0)
>>> +		btf_id = bp->id;
>>> +	else
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	if (btf_id > 0)
>>> +		t = btf_type_by_id(btf, btf_id);
>>> +	if (btf_id <= 0 || !t)
>>> +		return ptr_to_id(buf, end, bp->ptr, spec);
>>> +
>>> +	buf += btf_type_snprintf_show(btf, btf_id, bp->ptr, buf,
>>> +				      end - buf_start, flags);
>>> +
>>> +	return widen_string(buf, buf - buf_start, end, spec);
>>> +}
[...]