Re: [PATCH bpf-next v13 4/8] bpf: Add syscall common attributes support for prog_load

On 13/5/26 06:18, [email protected] wrote:
[...]
>>  int bpf_log_attr_init(struct bpf_log_attr *log, u64 log_buf, u32 log_size, u32 log_level,
>> -		      u32 offsetof_log_true_size, bpfptr_t uattr)
>> +		      u32 offsetof_log_true_size, bpfptr_t uattr, struct bpf_common_attr *common,
>> +		      bpfptr_t uattr_common, u32 size_common)
>>  {
>> +	char __user *ubuf_common = u64_to_user_ptr(common->log_buf);
>>  	char __user *ubuf = u64_to_user_ptr(log_buf);
>>  
>> +	if (!bpf_verifier_log_attr_valid(common->log_level, ubuf_common, common->log_size) ||
>> +	    !bpf_verifier_log_attr_valid(log_level, ubuf, log_size))
>> +		return -EINVAL;
>> +
>> +	if (ubuf && ubuf_common && (ubuf != ubuf_common || log_size != common->log_size ||
>> +				    log_level != common->log_level))
>> +		return -EINVAL;
> 
> If a user specifies log_level != 0 but leaves log_buf == NULL to probe for
> log_true_size, will these checks bypass conflicts?
> 
> If one side provides a complete tuple and the other provides only log_level,
> or if both provide mismatched log_levels without buffers, it appears the
> conflict checks are skipped and no -EINVAL is returned.
> 

Correct.

However, if users want to probe for log_true_size, they should not
provide common attrs. It is compatible for bpf libraries to probe for
log_true_size using union bpf_attr without providing common attrs.

>> +
>>  	memset(log, 0, sizeof(*log));
>>  	log->ubuf = ubuf;
>>  	log->size = log_size;
>>  	log->level = log_level;
>>  	log->offsetof_true_size = offsetof_log_true_size;
>>  	log->uattr = uattr;
>> +
>> +	if (!ubuf && ubuf_common) {
> 
> Does this condition silently ignore valid common->log_level configurations?
> 
> If the user provides only common->log_level via bpf_common_attr for a log
> size dry-run, ubuf_common evaluates to NULL. The condition here would
> evaluate to false, and common->log_level would be ignored instead of
> applied.
> 

The user should probe for log size using union bpf_attr instead of
common attrs.

> Also, if both specific and common attributes provide identical log
> parameters, the code appears to unconditionally use uattr for the
> log_true_size writeback since ubuf is not NULL.
> 
> If the original uattr uses an older layout without space for log_true_size,
> log->offsetof_true_size will be set to 0. Does this mean log_true_size
> writeback will be lost even if the user provided a modern attr_common
> struct with sufficient space to receive it?
> 

This was intentional.

If the user uses union bpf_attr to receive log, he should provide the
space for log_true_size.

IOW, if the user uses common attrs to receive log, he should provide the
space in common attrs for log_true_size.

Thanks,
Leon

>> +		log->ubuf = ubuf_common;
>> +		log->size = common->log_size;
>> +		log->level = common->log_level;
>> +		log->uattr = uattr_common;
>> +		log->offsetof_true_size = 0;
>> +		if (size_common >= offsetofend(struct bpf_common_attr, log_true_size))
>> +			log->offsetof_true_size = offsetof(struct bpf_common_attr, log_true_size);
>> +	}
>>  	return 0;
>>  }
>