netdev - Re: [PATCH net-next v2 6/9] devlink: support directly reading from region memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bd3887c2-1d91-3af1-f1cc-403b9ef849b7@intel.com>
Date:   Mon, 28 Nov 2022 10:34:52 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     <netdev@...r.kernel.org>, Jiri Pirko <jiri@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 6/9] devlink: support directly reading from
 region memory



On 11/24/2022 1:05 AM, Jiri Pirko wrote:
> Wed, Nov 23, 2022 at 09:38:31PM CET, jacob.e.keller@...el.com wrote:
> 
> Hmm. Being pedantic here, it changes userspace expectations:
> current - message without snapshotid is errored out
> new - message without snapshotid is happily processed
> 

Yea. I guess I'm thinking more from an interactive application 
standpoint this wouldn't be a problem but from a scripted setup it might 
do something weird.

> I can imagine some obscure userspace app depending on this behaviour,
> in theory.

Hmm.

> 
> Safe would be to add new NLA_FLAG attr DEVLINK_ATTR_REGION_DIRECT or
> something that would indicate userspace is interested in direct read.
> Also, the userspace would know right away it this new functionality
> is supported or not by the kernel.
> 

The advantage of being able to see that such a feature exists is good.

I can rework this to add an attribute.

> 
> 
>> +		if (!region->ops->read) {
>> +			NL_SET_ERR_MSG(cb->extack, "requested region does not support direct read");
>> +			err = -EOPNOTSUPP;
>> +			goto out_unlock;
>> +		}
>> +		if (port)
>> +			region_cb = &devlink_region_port_direct_fill;
>> +		else
>> +			region_cb = &devlink_region_direct_fill;
>> +		region_cb_priv = region;
>> +	} else {
>> +		struct devlink_snapshot *snapshot;
>> +		u32 snapshot_id;
>> +
>> +		snapshot_id = nla_get_u32(snapshot_attr);
>> +		snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>> +		if (!snapshot) {
>> +			NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");
>> +			err = -EINVAL;
>> +			goto out_unlock;
>> +		}
>> +		region_cb = &devlink_region_snapshot_fill;
>> +		region_cb_priv = snapshot;
>> 	}
>>
>> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
>> @@ -6633,9 +6665,9 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
>> 		goto nla_put_failure;
>> 	}
>>
>> -	err = devlink_nl_region_read_fill(skb, &devlink_region_snapshot_fill,
>> -					  snapshot, start_offset, end_offset,
>> -					  &ret_offset, cb->extack);
>> +	err = devlink_nl_region_read_fill(skb, region_cb, region_cb_priv,
>> +					  start_offset, end_offset, &ret_offset,
>> +					  cb->extack);
>>
>> 	if (err && err != -EMSGSIZE)
>> 		goto nla_put_failure;
>> -- 
>> 2.38.1.420.g319605f8f00e
>>