diff mbox series

[v2] ethdev: add sanity checks in control APIs

Message ID 1618284134-26152-1-git-send-email-humin29@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] ethdev: add sanity checks in control APIs |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot fail travis build: failed
ci/github-robot fail github build: failed
ci/Intel-compilation success Compilation OK
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-testing success Testing PASS

Commit Message

humin (Q) April 13, 2021, 3:22 a.m. UTC 
  This patch adds more sanity checks in control path APIs.

Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
Fixes: 0366137722a0 ("ethdev: check for invalid device name")
Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
Fixes: f8244c6399d9 ("ethdev: increase port id range")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* Removed unnecessary checks.
* Deleted checks in internal API.
* Added documentation in the header file.

---
 lib/librte_ethdev/rte_ethdev.c | 131 ++++++++++++++++++++++++++++++++++++++---
 lib/librte_ethdev/rte_ethdev.h |  21 ++++++-
 2 files changed, 140 insertions(+), 12 deletions(-)

Comments

Andrew Rybchenko April 13, 2021, 8:44 a.m. UTC | #1 
On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
> This patch adds more sanity checks in control path APIs.
> 
> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Fixes: f8244c6399d9 ("ethdev: increase port id range")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

Many thanks for working on it. Few notes below.

[snip]

> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 6b5cfd6..e1655b5 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
>  {
>  	int ret;
>  
> +	if (owner == NULL)
> +		return -EINVAL;
> +

Here and in many-many cases below I think the order of checks
is important in cases when different error codes are returned.
When there is no any very good reasons why arguments should
be checked in different order, arguments should be checked in
order specified in function prototype. In this cases (and many
cases below), port_id should be checked first.

In this particular case it means that the pointer check
should be done in a static helper function.

One more point is error logging in the case of failure.
Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
find out that some of messages should be made INFO or DEBUG.
Something like:
   RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
port_id);

I'm not 100% sure in format, but my requirements are:
 - log messages should be unique
 - log messages should be human readable (i.e. I'd avoid
   usage of function name)
 - log messages should provide enough information to understand
   what went wrong and provide context (basically it correlates
   with uniqueness requirement)

@Thomas, @Ferruh, what do you think? It would be good if we
reach an argement before mass changes are done?

> @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
>  
> +	if (queue_id >= dev->data->nb_tx_queues) {
> +		RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
> +			       dev->data->nb_tx_queues);
> +		return -EINVAL;
> +	}
> +

I'm not 100% sure that it is a control path.

>  	/* Call driver to free pending mbufs. */
>  	ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
>  					       free_cnt);

[snip]
Thomas Monjalon April 13, 2021, 8:58 a.m. UTC | #2 
13/04/2021 10:44, Andrew Rybchenko:
> On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
> > @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
> >  {
> >  	int ret;
> >  
> > +	if (owner == NULL)
> > +		return -EINVAL;
> > +
> 
> Here and in many-many cases below I think the order of checks
> is important in cases when different error codes are returned.
> When there is no any very good reasons why arguments should
> be checked in different order, arguments should be checked in
> order specified in function prototype. In this cases (and many
> cases below), port_id should be checked first.
> 
> In this particular case it means that the pointer check
> should be done in a static helper function.
> 
> One more point is error logging in the case of failure.
> Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
> find out that some of messages should be made INFO or DEBUG.
> Something like:
>    RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
> port_id);
> 
> I'm not 100% sure in format, but my requirements are:
>  - log messages should be unique
>  - log messages should be human readable (i.e. I'd avoid
>    usage of function name)
>  - log messages should provide enough information to understand
>    what went wrong and provide context (basically it correlates
>    with uniqueness requirement)
> 
> @Thomas, @Ferruh, what do you think? It would be good if we
> reach an argement before mass changes are done?

I agree with all your comments Andrew.


> > @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
> >  
> > +	if (queue_id >= dev->data->nb_tx_queues) {
> > +		RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
> > +			       dev->data->nb_tx_queues);
> > +		return -EINVAL;
> > +	}
> > +
> 
> I'm not 100% sure that it is a control path.

Indeed it can be used in the data path of some algorithms.
Ferruh Yigit April 13, 2021, 9:24 a.m. UTC | #3 
On 4/13/2021 9:58 AM, Thomas Monjalon wrote:
> 13/04/2021 10:44, Andrew Rybchenko:
>> On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
>>> @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
>>>   {
>>>   	int ret;
>>>   
>>> +	if (owner == NULL)
>>> +		return -EINVAL;
>>> +
>>
>> Here and in many-many cases below I think the order of checks
>> is important in cases when different error codes are returned.
>> When there is no any very good reasons why arguments should
>> be checked in different order, arguments should be checked in
>> order specified in function prototype. In this cases (and many
>> cases below), port_id should be checked first.
>>
>> In this particular case it means that the pointer check
>> should be done in a static helper function.
>>
>> One more point is error logging in the case of failure.
>> Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
>> find out that some of messages should be made INFO or DEBUG.
>> Something like:
>>     RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
>> port_id);
>>
>> I'm not 100% sure in format, but my requirements are:
>>   - log messages should be unique
>>   - log messages should be human readable (i.e. I'd avoid
>>     usage of function name)
>>   - log messages should provide enough information to understand
>>     what went wrong and provide context (basically it correlates
>>     with uniqueness requirement)
>>
>> @Thomas, @Ferruh, what do you think? It would be good if we
>> reach an argement before mass changes are done?
> 
> I agree with all your comments Andrew.
> 

+1

> 
>>> @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
>>>   
>>> +	if (queue_id >= dev->data->nb_tx_queues) {
>>> +		RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
>>> +			       dev->data->nb_tx_queues);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>
>> I'm not 100% sure that it is a control path.
> 
> Indeed it can be used in the data path of some algorithms.
> 
> 
>
humin (Q) April 14, 2021, 11:12 a.m. UTC | #4 
Hi, Thanks Andrew,
	All has been fixed in v3, please review it, thanks.

在 2021/4/13 16:44, Andrew Rybchenko 写道:
> On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
>> This patch adds more sanity checks in control path APIs.
>>
>> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
>> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
>> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
>> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
>> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
>> Fixes: f8244c6399d9 ("ethdev: increase port id range")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> Many thanks for working on it. Few notes below.
> 
> [snip]
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 6b5cfd6..e1655b5 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
>>   {
>>   	int ret;
>>   
>> +	if (owner == NULL)
>> +		return -EINVAL;
>> +
> 
> Here and in many-many cases below I think the order of checks
> is important in cases when different error codes are returned.
> When there is no any very good reasons why arguments should
> be checked in different order, arguments should be checked in
> order specified in function prototype. In this cases (and many
> cases below), port_id should be checked first.
> 
> In this particular case it means that the pointer check
> should be done in a static helper function.
> 
> One more point is error logging in the case of failure.
> Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
> find out that some of messages should be made INFO or DEBUG.
> Something like:
>     RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
> port_id);
> 
> I'm not 100% sure in format, but my requirements are:
>   - log messages should be unique
>   - log messages should be human readable (i.e. I'd avoid
>     usage of function name)
>   - log messages should provide enough information to understand
>     what went wrong and provide context (basically it correlates
>     with uniqueness requirement)
> 
> @Thomas, @Ferruh, what do you think? It would be good if we
> reach an argement before mass changes are done?
> 
>> @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
>>   
>> +	if (queue_id >= dev->data->nb_tx_queues) {
>> +		RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
>> +			       dev->data->nb_tx_queues);
>> +		return -EINVAL;
>> +	}
>> +
> 
> I'm not 100% sure that it is a control path.
> 
>>   	/* Call driver to free pending mbufs. */
>>   	ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
>>   					       free_cnt);
> 
> [snip]
> .
>
Tyler Retzlaff April 29, 2021, 5:48 p.m. UTC | #5 
On Tue, Apr 13, 2021 at 11:22:14AM +0800, Min Hu (Connor) wrote:
> This patch adds more sanity checks in control path APIs.
> 
> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Fixes: f8244c6399d9 ("ethdev: increase port id range")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---

this whole patch breaks abi since it returns new errno that were not
previously documented or returned. even if it is accepted it probably
should not be backported to stable.

it is entirely conceivable that you can have code that was calling these
functions and checking for specific return values where the new return
values will not be handled at all or improperly handled.

you can't just start emitting brand new errors or different errors for
the same input parameters.

thanks.
Stephen Hemminger April 29, 2021, 6:18 p.m. UTC | #6 
On Thu, 29 Apr 2021 10:48:34 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Apr 13, 2021 at 11:22:14AM +0800, Min Hu (Connor) wrote:
> > This patch adds more sanity checks in control path APIs.
> > 
> > Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
> > Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
> > Fixes: 0366137722a0 ("ethdev: check for invalid device name")
> > Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
> > Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> > Fixes: f8244c6399d9 ("ethdev: increase port id range")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > ---  
> 
> this whole patch breaks abi since it returns new errno that were not
> previously documented or returned. even if it is accepted it probably
> should not be backported to stable.
> 
> it is entirely conceivable that you can have code that was calling these
> functions and checking for specific return values where the new return
> values will not be handled at all or improperly handled.
> 
> you can't just start emitting brand new errors or different errors for
> the same input parameters.

In practice, checking for passing a NULL doesn't add a lot of value.
No program should ever do that, and if it did the crash that happens
when it dereferenced is as good as an assert() or an error return.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 6b5cfd6..e1655b5 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -199,6 +199,9 @@  rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
 	char *cls_str = NULL;
 	int str_size;
 
+	if (iter == NULL || devargs_str == NULL)
+		return -EINVAL;
+
 	memset(iter, 0, sizeof(*iter));
 
 	/*
@@ -293,7 +296,7 @@  rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str)
 uint16_t
 rte_eth_iterator_next(struct rte_dev_iterator *iter)
 {
-	if (iter->cls == NULL) /* invalid ethdev iterator */
+	if (iter == NULL || iter->cls == NULL) /* invalid ethdev iterator */
 		return RTE_MAX_ETHPORTS;
 
 	do { /* loop to try all matching rte_device */
@@ -322,7 +325,7 @@  rte_eth_iterator_next(struct rte_dev_iterator *iter)
 void
 rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
 {
-	if (iter->bus_str == NULL)
+	if (iter == NULL || iter->bus_str == NULL)
 		return; /* nothing to free in pure class filter */
 	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
 	free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
@@ -622,6 +625,9 @@  rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 int
 rte_eth_dev_owner_new(uint64_t *owner_id)
 {
+	if (owner_id == NULL)
+		return -EINVAL;
+
 	eth_dev_shared_data_prepare();
 
 	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
@@ -678,6 +684,9 @@  rte_eth_dev_owner_set(const uint16_t port_id,
 {
 	int ret;
 
+	if (owner == NULL)
+		return -EINVAL;
+
 	eth_dev_shared_data_prepare();
 
 	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
@@ -741,6 +750,9 @@  rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 	int ret = 0;
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 
+	if (owner == NULL)
+		return -EINVAL;
+
 	eth_dev_shared_data_prepare();
 
 	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
@@ -820,7 +832,7 @@  rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
 	uint16_t pid;
 
-	if (name == NULL) {
+	if (name == NULL || port_id == NULL) {
 		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
 		return -EINVAL;
 	}
@@ -1297,6 +1309,9 @@  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	int ret;
 	uint16_t old_mtu;
 
+	if (dev_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -2137,6 +2152,9 @@  rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	int i;
 	int count;
 
+	if (conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -2309,6 +2327,9 @@  rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 	int count;
 	int ret;
 
+	if (conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	if (tx_queue_id >= dev->data->nb_tx_queues) {
@@ -2459,6 +2480,9 @@  int
 rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
 		buffer_tx_error_fn cbfn, void *userdata)
 {
+	if (buffer == NULL)
+		return -EINVAL;
+
 	buffer->error_callback = cbfn;
 	buffer->error_userdata = userdata;
 	return 0;
@@ -2491,6 +2515,12 @@  rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
 
+	if (queue_id >= dev->data->nb_tx_queues) {
+		RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
+			       dev->data->nb_tx_queues);
+		return -EINVAL;
+	}
+
 	/* Call driver to free pending mbufs. */
 	ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
 					       free_cnt);
@@ -2606,6 +2636,9 @@  rte_eth_link_get(uint16_t port_id, struct rte_eth_link *eth_link)
 {
 	struct rte_eth_dev *dev;
 
+	if (eth_link == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -2626,6 +2659,9 @@  rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
 {
 	struct rte_eth_dev *dev;
 
+	if (eth_link == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -2667,6 +2703,9 @@  rte_eth_link_speed_to_str(uint32_t link_speed)
 int
 rte_eth_link_to_str(char *str, size_t len, const struct rte_eth_link *eth_link)
 {
+	if (str == NULL || eth_link == NULL)
+		return -EINVAL;
+
 	if (eth_link->link_status == ETH_LINK_DOWN)
 		return snprintf(str, len, "Link down");
 	else
@@ -2683,6 +2722,9 @@  rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
 {
 	struct rte_eth_dev *dev;
 
+	if (stats == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -3257,6 +3299,9 @@  rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
 {
 	struct rte_eth_dev *dev;
 
+	if (fw_version == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -3278,6 +3323,9 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 	};
 	int diag;
 
+	if (dev_info == NULL)
+		return -EINVAL;
+
 	/*
 	 * Init dev_info before port_id check since caller does not have
 	 * return status and does not know if get is successful or not.
@@ -3325,6 +3373,9 @@  rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
 	struct rte_eth_dev *dev;
 	const uint32_t *all_ptypes;
 
+	if (ptypes == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
@@ -3434,6 +3485,9 @@  rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr)
 {
 	struct rte_eth_dev *dev;
 
+	if (mac_addr == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	rte_ether_addr_copy(&dev->data->mac_addrs[0], mac_addr);
@@ -3446,6 +3500,9 @@  rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu)
 {
 	struct rte_eth_dev *dev;
 
+	if (mtu == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -3695,6 +3752,9 @@  rte_eth_dev_flow_ctrl_get(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
 {
 	struct rte_eth_dev *dev;
 
+	if (fc_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->flow_ctrl_get, -ENOTSUP);
@@ -3707,6 +3767,9 @@  rte_eth_dev_flow_ctrl_set(uint16_t port_id, struct rte_eth_fc_conf *fc_conf)
 {
 	struct rte_eth_dev *dev;
 
+	if (fc_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	if ((fc_conf->send_xon != 0) && (fc_conf->send_xon != 1)) {
 		RTE_ETHDEV_LOG(ERR, "Invalid send_xon, only 0/1 allowed\n");
@@ -3724,6 +3787,9 @@  rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (pfc_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	if (pfc_conf->priority > (ETH_DCB_NUM_USER_PRIORITIES - 1)) {
 		RTE_ETHDEV_LOG(ERR, "Invalid priority, only 0-7 allowed\n");
@@ -3744,9 +3810,6 @@  eth_check_reta_mask(struct rte_eth_rss_reta_entry64 *reta_conf,
 {
 	uint16_t i, num;
 
-	if (!reta_conf)
-		return -EINVAL;
-
 	num = (reta_size + RTE_RETA_GROUP_SIZE - 1) / RTE_RETA_GROUP_SIZE;
 	for (i = 0; i < num; i++) {
 		if (reta_conf[i].mask)
@@ -3763,9 +3826,6 @@  eth_check_reta_entry(struct rte_eth_rss_reta_entry64 *reta_conf,
 {
 	uint16_t i, idx, shift;
 
-	if (!reta_conf)
-		return -EINVAL;
-
 	if (max_rxq == 0) {
 		RTE_ETHDEV_LOG(ERR, "No receive queue is available\n");
 		return -EINVAL;
@@ -3795,6 +3855,9 @@  rte_eth_dev_rss_reta_update(uint16_t port_id,
 	struct rte_eth_dev *dev;
 	int ret;
 
+	if (reta_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	/* Check mask bits */
 	ret = eth_check_reta_mask(reta_conf, reta_size);
@@ -3822,6 +3885,9 @@  rte_eth_dev_rss_reta_query(uint16_t port_id,
 	struct rte_eth_dev *dev;
 	int ret;
 
+	if (reta_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	/* Check mask bits */
@@ -3843,6 +3909,9 @@  rte_eth_dev_rss_hash_update(uint16_t port_id,
 	struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
 	int ret;
 
+	if (rss_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
@@ -3871,6 +3940,9 @@  rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (rss_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
@@ -4026,6 +4098,9 @@  rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
 	uint64_t pool_mask;
 	int ret;
 
+	if (addr == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
@@ -4076,6 +4151,9 @@  rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
 	struct rte_eth_dev *dev;
 	int index;
 
+	if (addr == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
@@ -4107,6 +4185,9 @@  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 	struct rte_eth_dev *dev;
 	int ret;
 
+	if (addr == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	if (!rte_is_valid_assigned_ether_addr(addr))
@@ -4162,6 +4243,9 @@  rte_eth_dev_uc_hash_table_set(uint16_t port_id, struct rte_ether_addr *addr,
 	int ret;
 	struct rte_eth_dev *dev;
 
+	if (addr == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -4264,6 +4348,9 @@  rte_eth_mirror_rule_set(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (mirror_conf == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	if (mirror_conf->rule_type == 0) {
 		RTE_ETHDEV_LOG(ERR, "Mirror rule type can not be 0\n");
@@ -4602,6 +4689,9 @@  rte_eth_dma_zone_free(const struct rte_eth_dev *dev, const char *ring_name,
 	const struct rte_memzone *mz;
 	int rc = 0;
 
+	if (dev == NULL || ring_name == NULL)
+		return -EINVAL;
+
 	rc = eth_dev_dma_mzone_name(z_name, sizeof(z_name), dev->data->port_id,
 			queue_id, ring_name);
 	if (rc >= RTE_MEMZONE_NAMESIZE) {
@@ -5207,6 +5297,9 @@  rte_eth_timesync_read_rx_timestamp(uint16_t port_id, struct timespec *timestamp,
 {
 	struct rte_eth_dev *dev;
 
+	if (timestamp == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -5221,6 +5314,9 @@  rte_eth_timesync_read_tx_timestamp(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (timestamp == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -5247,6 +5343,9 @@  rte_eth_timesync_read_time(uint16_t port_id, struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
 
+	if (timestamp == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -5260,6 +5359,9 @@  rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *timestamp)
 {
 	struct rte_eth_dev *dev;
 
+	if (timestamp == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -5273,6 +5375,9 @@  rte_eth_read_clock(uint16_t port_id, uint64_t *clock)
 {
 	struct rte_eth_dev *dev;
 
+	if (clock == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
@@ -5370,6 +5475,9 @@  rte_eth_dev_get_dcb_info(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (dcb_info == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -5421,6 +5529,9 @@  rte_eth_dev_hairpin_capability_get(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
+	if (cap == NULL)
+		return -EINVAL;
+
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
@@ -5629,6 +5740,8 @@  rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
 	struct rte_eth_representor_info *info = NULL;
 	size_t size;
 
+	if (ethdev == NULL)
+		return -EINVAL;
 	if (type == RTE_ETH_REPRESENTOR_NONE)
 		return 0;
 	if (repr_id == NULL)
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 3b773b6..5c03e9a 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2702,6 +2702,7 @@  int rte_eth_allmulticast_get(uint16_t port_id);
  *   - (0) if successful.
  *   - (-ENOTSUP) if the function is not supported in PMD driver.
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
 
@@ -2717,6 +2718,7 @@  int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
  *   - (0) if successful.
  *   - (-ENOTSUP) if the function is not supported in PMD driver.
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *link);
 
@@ -2752,7 +2754,7 @@  const char *rte_eth_link_speed_to_str(uint32_t link_speed);
  * @param eth_link
  *   Link status returned by rte_eth_link_get function
  * @return
- *   Number of bytes written to str array.
+ *   Number of bytes written to str array or -EINVAL if bad parameter.
  */
 __rte_experimental
 int rte_eth_link_to_str(char *str, size_t len,
@@ -2997,6 +2999,7 @@  int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
  * @return
  *   - (0) if successful
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
 
@@ -3041,6 +3044,7 @@  int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
  *   - (0) if successful.
  *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
 
@@ -3060,6 +3064,7 @@  int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
  *   - (-ENOTSUP) if operation is not supported.
  *   - (-ENODEV) if *port_id* invalid.
  *   - (-EIO) if device is removed.
+ *   - (-EINVAL) if bad parameter.
  *   - (>0) if *fw_size* is not enough to store firmware version, return
  *          the size of the non truncated string.
  */
@@ -3103,6 +3108,7 @@  int rte_eth_dev_fw_version_get(uint16_t port_id,
  *           only num entries will be filled into the ptypes array, but the full
  *           count of supported ptypes will be returned.
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
 				     uint32_t *ptypes, int num);
@@ -3153,6 +3159,7 @@  int rte_eth_dev_set_ptypes(uint16_t port_id, uint32_t ptype_mask,
  * @return
  *   - (0) if successful.
  *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_get_mtu(uint16_t port_id, uint16_t *mtu);
 
@@ -3347,7 +3354,7 @@  rte_eth_tx_buffer_init(struct rte_eth_dev_tx_buffer *buffer, uint16_t size);
  * @param userdata
  *   Arbitrary parameter to be passed to the callback function
  * @return
- *   0 on success, or -1 on error with rte_errno set appropriately
+ *   0 on success, or -EINVAL if bad parameter
  */
 int
 rte_eth_tx_buffer_set_err_callback(struct rte_eth_dev_tx_buffer *buffer,
@@ -3427,6 +3434,7 @@  rte_eth_tx_buffer_count_callback(struct rte_mbuf **pkts, uint16_t unsent,
  *     -ENODEV: Invalid interface
  *     -EIO: device is removed
  *     -ENOTSUP: Driver does not support function
+ *     -EINVAL: Bad parameter
  *   Success: >= 0
  *     0-n: Number of packets freed. More packets may still remain in ring that
  *     are in use.
@@ -3774,6 +3782,7 @@  int rte_eth_fec_set(uint16_t port_id, uint32_t fec_capa);
  *   - (-ENOTSUP) if hardware doesn't support flow control.
  *   - (-ENODEV)  if *port_id* invalid.
  *   - (-EIO)  if device is removed.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_flow_ctrl_get(uint16_t port_id,
 			      struct rte_eth_fc_conf *fc_conf);
@@ -3845,7 +3854,8 @@  int rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *mac_addr,
  *   - (0) if successful, or *mac_addr* didn't exist.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-ENODEV) if *port* invalid.
- *   - (-EADDRINUSE) if attempting to remove the default MAC address
+ *   - (-EADDRINUSE) if attempting to remove the default MAC address.
+ *   - (-EINVAL) if MAC address is invalid.
  */
 int rte_eth_dev_mac_addr_remove(uint16_t port_id,
 				struct rte_ether_addr *mac_addr);
@@ -4044,6 +4054,7 @@  int rte_eth_dev_rss_hash_update(uint16_t port_id,
  *   - (-ENODEV) if port identifier is invalid.
  *   - (-EIO) if device is removed.
  *   - (-ENOTSUP) if hardware doesn't support RSS.
+ *   - (-EINVAL) if bad parameter.
  */
 int
 rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
@@ -4112,6 +4123,7 @@  rte_eth_dev_udp_tunnel_port_delete(uint16_t port_id,
  *   - (-ENODEV) if port identifier is invalid.
  *   - (-EIO) if device is removed.
  *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-EINVAL) if bad parameter.
  */
 int rte_eth_dev_get_dcb_info(uint16_t port_id,
 			     struct rte_eth_dcb_info *dcb_info);
@@ -4628,6 +4640,7 @@  int rte_eth_timesync_adjust_time(uint16_t port_id, int64_t delta);
  *
  * @return
  *   - 0: Success.
+ *   - -EINVAL: Bad parameter.
  */
 int rte_eth_timesync_read_time(uint16_t port_id, struct timespec *time);
 
@@ -4694,6 +4707,7 @@  int rte_eth_timesync_write_time(uint16_t port_id, const struct timespec *time);
  *   - 0: Success.
  *   - -ENODEV: The port ID is invalid.
  *   - -ENOTSUP: The function is not supported by the Ethernet driver.
+ *   - -EINVAL: if bad parameter.
  */
 __rte_experimental
 int
@@ -4797,6 +4811,7 @@  rte_eth_dev_get_sec_ctx(uint16_t port_id);
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-EINVAL) if bad parameter.
  */
 __rte_experimental
 int rte_eth_dev_hairpin_capability_get(uint16_t port_id,