Re: [PATCH 17/21] fastboot net: implement fastboot over UDP

["Daniel Glöckner" <dg@emlix.com>]

Hello Sascha,

Am 19.06.20 um 09:44 schrieb Sascha Hauer:
> +struct fastboot_net {
> +	struct fastboot fastboot;
> +
> +	struct net_connection *net_con;
> +	struct fastboot_header response_header;
> +	struct poller_struct poller;
> +	struct work_queue wq;
> +	u64 host_waits_since;
> +	u64 last_download_pkt;
> +	bool sequence_number_seen;
> +	bool active_download;
> +	bool reinit;
> +	bool send_keep_alive;
> +	enum may_send may_send;
> +
> +	IPaddr_t host_addr;
> +	u16 host_port;
> +	u8 host_mac[ETH_ALEN];
> +	u16 sequence_number;
> +	u16 last_payload_len;
> +	uchar last_payload[FASTBOOT_MAX_CMD_LEN + sizeof(struct fastboot_header)];

This is not FASTBOOT_MAX_CMD_LEN. It's the 64 that is strewn around in
fastboot_tx_print. Adding a new constant FASTBOOT_MAX_MSG_LEN would be
correct.

[...]

> +static int fastboot_write_net(struct fastboot *fb, const char *buf,
> +			      unsigned int n)
> +{
> +	struct fastboot_net *fbn = container_of(fb, struct fastboot_net,
> +						fastboot);
> +	struct fastboot_header response_header;
> +	uchar *packet;
> +	uchar *packet_base;
> +	int ret;
> +
> +	if (fbn->reinit)
> +		return 0;
> +
> +	/*
> +	 * This function is either called in command context, in which
> +	 * case we may wait, or from the keepalive poller which explicitly
> +	 * only calls us when we don't have to wait here.
> +	 */
> +	ret = fastboot_net_wait_may_send(fbn);
> +	if (ret) {
> +		fastboot_net_abort(fbn);
> +		return ret;
> +	}
> +
> +	if (n && fbn->may_send == MAY_SEND_ACK) {
> +		fastboot_send(fbn, fbn->response_header,
> +				"Have message but only ACK allowed");
> +		return -EPROTO;
> +	} else if (!n && fbn->may_send == MAY_SEND_MESSAGE) {
> +		fastboot_send(fbn, fbn->response_header,
> +				"Want to send ACK but message expected");
> +		return -EPROTO;
> +	}
> +
> +	response_header = fbn->response_header;
> +	response_header.flags = 0;
> +	response_header.seq = htons(fbn->sequence_number);
> +	++fbn->sequence_number;
> +	fbn->sequence_number_seen = false;
> +
> +	packet = net_udp_get_payload(fbn->net_con);
> +	packet_base = packet;
> +
> +	/* Write headers */
> +	memcpy(packet, &response_header, sizeof(response_header));
> +	packet += sizeof(response_header);
> +	/* Write response */
> +	memcpy(packet, buf, n);
> +	packet += n;
> +
> +	/* Save packet for retransmitting */
> +	fbn->last_payload_len = packet - packet_base;
> +	memcpy(fbn->last_payload, packet_base, fbn->last_payload_len);
> +
> +	memcpy(fbn->net_con->et->et_dest, fbn->host_mac, ETH_ALEN);
> +	net_write_ip(&fbn->net_con->ip->daddr, fbn->host_addr);
> +	fbn->net_con->udp->uh_dport = fbn->host_port;
> +	net_udp_send(fbn->net_con, fbn->last_payload_len);
> +
> +	fbn->may_send = MAY_NOT_SEND;

You moved that line below net_udp_send. Is there any risk that

1. our work queue executes a command which calls fastboot_tx_print
2. the net_udp_send caused by that fastboot_tx_print sleeps
3. our poller is executed and decides to send a message because
   may_send is still MAY_SEND_MESSAGE

?

[...]

> +static void fastboot_start_download_net(struct fastboot *fb)
> +{
> +	struct fastboot_net *fbn = container_of(fb, struct fastboot_net,
> +						fastboot);
> +
> +	fastboot_start_download_generic(fb);
> +	fbn->active_download = true;
> +	fbn->last_download_pkt = get_time_ns();
> +}

Although you added that last_download_pkt timeout check to the poller,
there is still the risk that we will never close download_fd if
fastboot_net_abort is called (f.ex. by the first fastboot_tx_print
inside cb_download) before we open download_fd. In that case there
is no poller to check for the timeout.

[...]

> +static void fastboot_handle_type_fastboot(struct fastboot_net *fbn,
> +					  struct fastboot_header header,
> +					  char *fastboot_data,
> +					  unsigned int fastboot_data_len)
> +{
> +	struct fastboot_work *w;
> +
> +	fbn->response_header = header;
> +	fbn->host_waits_since = get_time_ns();
> +	fbn->may_send = fastboot_data_len ? MAY_SEND_ACK : MAY_SEND_MESSAGE;
> +
> +	if (fbn->active_download) {
> +		fbn->last_download_pkt = get_time_ns();
> +
> +		if (!fastboot_data_len && fbn->fastboot.download_bytes
> +					   == fbn->fastboot.download_size) {
> +
> +			fbn->active_download = false;
> +
> +			w = xzalloc(sizeof(*w));
> +			w->fbn = fbn;
> +			w->download_finished = true;
> +
> +			wq_queue_work(&fbn->wq, &w->work);
> +		} else {
> +			fastboot_data_download(fbn, fastboot_data,
> +					       fastboot_data_len);
> +		}
> +		return;
> +	}
> +
> +	if (fastboot_data_len >= FASTBOOT_MAX_CMD_LEN) {

Still off-by-one. Replace >= with >

[...]

> +	case FASTBOOT_INIT:
> +		if (ntohs(header.seq) != fbn->sequence_number) {
> +			fastboot_check_retransmit(fbn, header);
> +			break;
> +		}
> +		fbn->host_addr = net_read_ip(&ip_header->saddr);
> +		fbn->host_port = udp_header->uh_sport;
> +		memcpy(fbn->host_mac, eth_header->et_src, ETH_ALEN);
> +		fastboot_net_abort(fbn);
> +		ret = poller_register(&fbn->poller, "fastboot");
> +		if (ret) {
> +			pr_err("Cannot register poller: %s\n", strerror(-ret));
> +			return;

It is not obvious that a second FASTBOOT_INIT will _not_ cause this
error because fastboot_net_abort unregistered the previous poller.
I would at least add a comment to the fastboot_net_abort(fbn) line.

[...]

> +static void fastboot_poll(struct poller_struct *poller)
> +{
> +	struct fastboot_net *fbn = container_of(poller, struct fastboot_net,
> +					       poller);
> +
> +	if (fbn->active_download && is_timeout(fbn->last_download_pkt, 5 * SECOND)) {

Should pollers prefer is_timeout_non_interruptible over is_timeout?

I can make a new patch set where all issues are fixed, if you don't insist
on doing it yourself.

Best regards,

  Daniel


-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055

emlix - your embedded linux partner

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox